-
Notifications
You must be signed in to change notification settings - Fork 13.2k
fix: imports/exports/require with template literal tokens #36546
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: imports/exports/require with template literal tokens #36546
Conversation
src/services/preProcess.ts
Outdated
| if (lastToken === SyntaxKind.DotToken | ||
| // the following is used to prevent matches of | ||
| // `import * as doh from 'ooops';\n`; | ||
| // however this is only best-effort as it is possible to have a template | ||
| // literal ending right before, not only opening, which would prevent this | ||
| // import from being detected. | ||
| || lastToken === SyntaxKind.NoSubstitutionTemplateLiteral) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be rolled back with #36143
| // https://github.com/microsoft/TypeScript/issues/30878#issuecomment-540698189 | ||
| it.skip("Do not return reference path of imports in string literals", () => { | ||
| test("`${}`;\n`import * as doh from 'ooops';\n`;", | ||
| /*readImportFile*/ true, | ||
| /*detectJavaScriptImports*/ false, | ||
| { | ||
| referencedFiles: <ts.FileReference[]>[], | ||
| importedFiles: <ts.FileReference[]>[], | ||
| typeReferenceDirectives: [], | ||
| libReferenceDirectives: [], | ||
| ambientExternalModules: undefined, | ||
| isLibFile: false | ||
| }); | ||
| }); | ||
|
|
||
| // https://github.com/microsoft/TypeScript/issues/30878#issue-432369315 | ||
| it.skip("Correctly return statically imported files after string literals", () => { | ||
| test("`${foo}`; import './r1.ts';", | ||
| /*readImportFile*/ true, | ||
| /*detectJavaScriptImports*/ false, | ||
| { | ||
| referencedFiles: <ts.FileReference[]>[], | ||
| typeReferenceDirectives: [], | ||
| libReferenceDirectives: [], | ||
| importedFiles: [{ fileName: "r1.ts", pos: 17, end: 24 }], | ||
| ambientExternalModules: undefined, | ||
| isLibFile: false | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should pass with #36143
|
|
||
| // https://github.com/microsoft/TypeScript/issues/33680#issue-500399194 | ||
| it("Correctly return static imports using string literals", () => { | ||
| test("import d from `r1.ts`; import d, {a, b as B} from `r2.ts`; import d, * as NS from `r3.ts`; export {a, b as B} from `r4.ts`; export * from `r5.ts`;", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not valid JS/TS (#29318) should it be parsed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the only thing blocking the rest of the PR I am happy to roll the change back and open it in a separate request where we can then have the discussion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then how's different from #33688 with previous roll back removed string literal detection removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logically they are very similar - I don't completely understand the reason for the introduction of https://github.com/microsoft/TypeScript/pull/33688/files#diff-23f96e78fc240e5f0198a9ffef3a69e4R227; however, I believe the way I structured the tests, updated the comments and implemented the union for literal likes is more in line with the rest of the codebase. When I opened the PR unfortunately I didn't see #33688, otherwise I could have based it on top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because import mod = require(`mod`); is error in TS String literal expected. (Spec)
They have same validation as import mod from `mod`;
|
This needs to be closed since import a from |
Backlogmilestone (required)masterbranchgulp runtestslocallyFixes #33680
Complements #36143
Related to #30878