-
Notifications
You must be signed in to change notification settings - Fork 763
Fix empty wildcard match on exports #2264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix empty wildcard match on exports #2264
Conversation
|
@microsoft-github-policy-service agree |
tsgo will crash without microsoft/typescript-go#2264 Additional tweaks are required for typescript language changes. There are additional semantic errors (I assume bugs) that affect the tsgo CLI but not language server that also need further investigation.
tsgo will crash without microsoft/typescript-go#2264 Additional tweaks are required for typescript language changes. There are additional semantic errors (I assume bugs) that affect the tsgo CLI but not language server that also need further investigation.
|
Is there a test you can make for this? The fix seems obviously right, however. The original code looked more like: case MatchingMode.Pattern:
const starPos = pathOrPattern.indexOf("*");
const leadingSlice = pathOrPattern.slice(0, starPos);
const trailingSlice = pathOrPattern.slice(starPos + 1);
if (canTryTsExtension && startsWith(targetFilePath, leadingSlice, ignoreCase) && endsWith(targetFilePath, trailingSlice, ignoreCase)) {
const starReplacement = targetFilePath.slice(leadingSlice.length, targetFilePath.length - trailingSlice.length);
return { moduleFileToTry: replaceFirstStar(packageName, starReplacement) };
}Which we could technically replicate? |
|
@jakebailey I did my best to add coverage in Interestingly the original code looks like it may be subtly incorrect as well. It won't crash due to JS's more lenient slice() but it does have the false positive for an overlapping prefix/suffix which could (maybe?) result in a match against an unintended file. |
Hm, I assumed not, given it splits the string before checking? |
Head branch was pushed to by a user without write access
The problem is the independent tests against startsWith(targetFilePath, leadingSlice, ignoreCase) && endsWith(targetFilePath, trailingSlice, ignoreCase)in my example above this ends up as something like: startsWith("foo/index.js", "foo/") && endsWith("foo/index.js", "/index.js")So "foo/*/index.js" unintentionally matches "foo/index.js". Not sure if there's a non-contrived scenario where the output could cause problems though... In our case the output references a module name containing "//" so is innocuous. |
This fixes a bug whereby a wildcard package.json export such as:
when matched against an import such as:
would crash: