Fix import addition location#17327
Conversation
| } | ||
| // As well as any triple slash references | ||
| for (const range of ranges) { | ||
| if (range.kind === SyntaxKind.SingleLineCommentTrivia && (node as SourceFile).text.substring(range.pos, range.pos + 3) === "///") { |
There was a problem hiding this comment.
I think we have an isDirectiveComment or something
There was a problem hiding this comment.
I'll search around for it later. I think its private to one of the emitters right now.
|
@DanielRosenwasser would you update your review status? |
paulvanbrenk
left a comment
There was a problem hiding this comment.
What happens when there is no whitespace between the pinned comment, and the attached comment? (And/Or a /// reference)?
…ode start; accept new baselines
|
@paulvanbrenk It should only skip triple-slash and pinned comments now, but lemme add a test for a file with all 3. Also, with |
| /** | ||
| * Copy with new flags via RegExp constructor is ES6+, so this needs to be manually synced with the above | ||
| */ | ||
| const defaultLibReferenceRegExGIM = /^(\/\/\/\s*<reference\s+no-default-lib\s*=\s*)('|")(.+?)\2\s*\/>/gim; |
There was a problem hiding this comment.
new RegExp(defaultLibReferenceRegEx.source, "gim") is valid in ES5.
There was a problem hiding this comment.
Should I use that within getFileReferenceFromReferencePath, instead, then? That aligns with the intent, so it seems good.
| export function getFileReferenceFromReferencePath(comment: string, commentRange: CommentRange): ReferencePathMatchResult { | ||
| const simpleReferenceRegEx = /^\/\/\/\s*<reference\s+/gim; | ||
| const isNoDefaultLibRegEx = /^(\/\/\/\s*<reference\s+no-default-lib\s*=\s*)('|")(.+?)\2\s*\/>/gim; | ||
| const isNoDefaultLibRegEx = new RegExp(defaultLibReferenceRegExGIM); |
There was a problem hiding this comment.
You could also do defaultLibReferenceRegExGIM.lastIndex = 0;
| export let fullTripleSlashReferencePathRegEx = /^(\/\/\/\s*<reference\s+path\s*=\s*)('|")(.+?)\2.*?\/>/; | ||
| export let fullTripleSlashReferenceTypeReferenceDirectiveRegEx = /^(\/\/\/\s*<reference\s+types\s*=\s*)('|")(.+?)\2.*?\/>/; | ||
| export let fullTripleSlashAMDReferencePathRegEx = /^(\/\/\/\s*<amd-dependency\s+path\s*=\s*)('|")(.+?)\2.*?\/>/; | ||
| export let defaultLibReferenceRegEx = /^(\/\/\/\s*<reference\s+no-default-lib\s*=\s*)('|")(.+?)\2\s*\/>/; |
There was a problem hiding this comment.
Does this need to be exported?
There was a problem hiding this comment.
We export the regexes for the rest of our matched triple-slash references, so I would assume so. Our triple-slash recognition functions are also currently all internal (prior to this PR anyway), so people need to replicate these regexes themselves if they're trying to emulate our behavior.
* Add test with bug * Fix for import placement * Consolidate comment recognition functions into utilities * Add another test with all 3 kinds * Recognize path directives as part of triple slash directives * Also handle no-default-lib triple-slash comments * Test for all the triple-slash kinds * Keep import-placement logic in the quickfix, since its not really a node start; accept new baselines * Work in not-ES6, use a real no-lib comment * Remove no default lib triple slash comment, it disables checking and thereby quick fixes * Copy regex rather than have a regex copy
Fixes #17318
We now distinguish between pinned and jsdoc comments (and triple slash references) when looking for the start of the file for this quickfix. Namely, we not assume that jsdoc comments are meant to be after the import unless pinned.