Add autoImportSpecifierExcludeRegexes preference#59543
Add autoImportSpecifierExcludeRegexes preference#59543andrewbranch merged 5 commits intomicrosoft:mainfrom
autoImportSpecifierExcludeRegexes preference#59543Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
andrewbranch
changed the title
autoImportSpecifierExcludeRegexes preference
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
| } | ||
|
|
||
| function isExcludedByRegex(moduleSpecifier: string, excludeRegexes: readonly string[] | undefined): boolean { | ||
| return some(excludeRegexes, pattern => !!stringToRegex(pattern)?.test(moduleSpecifier)); |
There was a problem hiding this comment.
wont this keep memoized map forever in tsserver - think you would want to memoize this and keep lifetime same as excludeRegex or something like that ?
There was a problem hiding this comment.
Yes, but I think normally the preference won’t change (or won’t change much) during the life of the server. It might be nice not to have to redo this on every request. Theoretically this could waste memory, but it will only have an impact if the user is changing between thousands of unique strings...
src/compiler/moduleSpecifiers.ts
Outdated
|
|
||
| const stringToRegex = memoizeOne((pattern: string) => { | ||
| try { | ||
| return new RegExp(pattern); |
There was a problem hiding this comment.
Do we have any other settings which are controlled by a regex like this? Trying to decide if people expect to be able to set it to a literal like "/foo/i" or something, but maybe that's a bit much.
That and, should this use getRegexFromPattern and handle case insensitive paths?
There was a problem hiding this comment.
We don’t, but C Spell does seem to allow slashes and flags. I’m not opposed to implementing that. Without the ability to provide flags, I figured case sensitive was the best option.
There was a problem hiding this comment.
Yeah, I think people can already do like (?:i) at the start of their patterns too?
I don't know how stable this option is or if you're just experimenting, though. That and I sort of bet people will want to write patterns that start with /...
There was a problem hiding this comment.
(?:i) isn't supported in js yet afaik so maybe better to require the whole /.../. Requires a little extra parsing but should be more explicit that this is a regex too
There was a problem hiding this comment.
Ah, yeah, I'm thinking of re2/Go syntax.
Other places that take regexes are git.branchValidationRegex in vscode, which just passes it into new RegExp https://github.com/microsoft/vscode/blob/8f88d203d76dee8c92e491f8a1e1331d74792b42/extensions/git/src/commands.ts#L2679
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
Looks good, with the caveat that I don't know this area very well.
One question: since autoImportFileExcludePatterns already exists, do these preferences always show up in a way that makes people aware that both exist? Eg in a graphical config UI next to each other, or in completions for the prefix "autoImport|". If that's not the case, I'd be worried that people would discover one but not the other.
|
@sandersn yes, they will, after the corresponding VS Code PR. |
|
not sure if it is worth opening separate issue as it might already been discussed. Would it be an option to improve this feature to allow specifying for which files the exclusion apply? In other words, as an example, while in |
|
I’m assuming you’re using relative imports within |
8 participants
Closes #55092
This has the limitation that it doesn't compose very intelligently with the
importModuleSpecifierEndingpreference—you can set that to"js"and then write a regex that excludes\.js$and the result will be that you get no (relative) auto imports at all. But I think I'd like to try this and get feedback.