Exclude legacy safelist files in external projects #19542
Conversation
src/server/editorServices.ts
Outdated
| const inferredTypingName = removeFileExtension(baseName); | ||
| const cleanedTypingName = removeMinAndVersionNumbers(inferredTypingName); | ||
| if (this.legacySafelist[cleanedTypingName]) { | ||
| this.logger.info(`Excluded '${normalizedNames[i]}'`); |
There was a problem hiding this comment.
this.logger.info(
Excluded '${normalizedNames[i]}'); [](start = 32, length = 53)
I think it would be helpful (and consistent) to indicate why the file is being excluded (i.e. legacy safelist).
|
Can you also add a test. |
src/compiler/core.ts
Outdated
| * Takes a string like "jquery-min.4.2.3" and returns "jquery" | ||
| */ | ||
| export function removeMinAndVersionNumbers(fileName: string) { | ||
| return fileName.replace(/((?:\.|-)min(?=\.|$))|((?:-|\.)\d+)/g, ""); |
There was a problem hiding this comment.
This also removes digits without the min, for example "jquery.4.2-test" also becomes "jquery-test". Probably not intended. (Also why use non-capturing groups (i.e. (?: )? You are not using the groups for anything, so it's just syntactic noise).
There was a problem hiding this comment.
What exactly should the rule be? Remove all characters after the first . if the thing after the . is min or a number?
There was a problem hiding this comment.
@DanielRosenwasser wrote us a nice regex
src/compiler/core.ts
Outdated
| @@ -2417,7 +2417,13 @@ namespace ts { | |||
| * Takes a string like "jquery-min.4.2.3" and returns "jquery" | |||
| */ | |||
| export function removeMinAndVersionNumbers(fileName: string) { | |||
| return fileName.replace(/((?:\.|-)min(?=\.|$))|((?:-|\.)\d+)/g, ""); | |||
| const match = /((\w|(-(?!min)))+)(\.|-)?.*/.exec(fileName); | |||
There was a problem hiding this comment.
Doesn't this also take something like "foo.bar.js", and turn it into just "foo.js"? Is that intended?
There was a problem hiding this comment.
I just whipped up the below, which seems to work. Take a look and see what you think.
Note: I don't think we need to worry about .jsx files being library files, or trying to handle suffixes like "-beta" after the min or version.
function removeMinAndVersionNumbers(path) {
// Match a "." or "-" followed by a version number or 'min', that occurs right before the final ".js"
let trailingMinOrVersion = /[.-]((min)|(\d+(\.\d+)*))\.js$/;
// The "min" or version may both be present, in either order, so try applying the above twice.
// Note the final ".js" is part of the match, so add this back as the substitution.
let result = path.replace(trailingMinOrVersion, ".js").replace(trailingMinOrVersion, ".js");
return result;
}There was a problem hiding this comment.
This doesn't handle jquery.6.min.js; not sure how common that is
There was a problem hiding this comment.
Yes it does. I just tried it in the Console.
removeMinAndVersionNumbers("jquery.6.min.js");
"jquery.js"
| /** | ||
| * Takes a string like "jquery-min.4.2.3" and returns "jquery" | ||
| */ | ||
| export function removeMinAndVersionNumbers(fileName: string) { |
There was a problem hiding this comment.
Do we really need this function in compiler? Can it be part of services\utilities instead?
There was a problem hiding this comment.
This functions needs to be used in the typings installer and in tsserver; I couldn't find another file that was shared by them. services\utilities isn't involved in the typings installer and adding it introduced more errors because it needs references to e.g. Project
| ["jquery-min.4.2.3", "jquery"], | ||
| // ["jquery.4.2-test.js", "jquery"], | ||
| ["jquery.min.4.2.1", "jquery"], | ||
| // ["jquery.7.min.js", "jquery"], |
There was a problem hiding this comment.
This is pretty much the same format as the case you thought didn't work. Any reason not to include it? (without the .js, if that's already removed by the time it gets here).
Also, why leave commented out tests? It looks like they should pass but are currently failing. We should just remove them if that is not the behavior we want (or enable them and change the expectations to the correct behavior).
Fixes half of #19533