fix inconsistencies in import UMD code fixes adapting to module format #19572
Conversation
| const { module, allowSyntheticDefaultImports } = context.compilerOptions; | ||
|
|
||
| // Prefer to import as a synthetic `default` if available. | ||
| if (allowSyntheticDefaultImports || module === ModuleKind.System && allowSyntheticDefaultImports !== false) { |
There was a problem hiding this comment.
Add a function to src\core.ts to getAllowSyntheticDefaultImports(compilerOptions) that has this logic, and share it with the checker as well.
| @@ -644,6 +665,19 @@ namespace ts.codefix { | |||
| Debug.fail("Either the symbol or the JSX namespace should be a UMD global if we got here"); | |||
| } | |||
|
|
|||
| const { module, allowSyntheticDefaultImports } = context.compilerOptions; | |||
There was a problem hiding this comment.
consider moving compilerOptions to top destructuring declaration.
| } | ||
|
|
||
| // When a synthetic `default` is unavailable, use `import..require` if the module kind supports it. | ||
| if (module === ModuleKind.AMD || module === ModuleKind.CommonJS || module === ModuleKind.UMD) { |
There was a problem hiding this comment.
not sure i agree we need this change.
There was a problem hiding this comment.
The argument here is that if require is available, it is a reliable away to obtain module.exports even when it is callable. I am happy to remove this as requested but believe it is valuable.
There was a problem hiding this comment.
This change is actually what's causing the build failures because of expectations in the four slash tests.
Would you like me to remove the require changes? I'd be happy to open a separate issue for them.
|
For tests, take a look at tests/cases/fourslash/importNameCodeFixNewImportFileDetachedComments.ts |
aluanhaddad
force-pushed
the
module-format-aware-import-fixes
branch
from
266ea5f
to
af6b43a
Compare
- use default import under --allowSyntheticDefaultImports - import..require support - make make quick fix info match resulting import - make diagnostics
- extract test for synethetic default imports into getAllowSyntheticDefaultImports in core.ts - use getAllowSyntheticDefaultImports in checker.ts and importFixes.ts - move compilerOptions to top level destructuring
aluanhaddad
force-pushed
the
module-format-aware-import-fixes
branch
from
06cd3c3
to
1be8b21
Compare
| const allowSyntheticDefaultImports = getAllowSyntheticDefaultImports(compilerOptions); | ||
|
|
||
| // Prefer to import as a synthetic `default` if available. | ||
| if (allowSyntheticDefaultImports) { |
There was a problem hiding this comment.
@weswigham this is another place for your new change in #19675 to be plugged in. we would like to always show the default import in this case and never fallback to import .. = require
There was a problem hiding this comment.
Roger. Thank you!
|
@aluanhaddad can you please address the test failures. |
|
I updated it to always uses the synthetic defaults for UMD modules unless |
|
thanks @aluanhaddad! Want to redo the |
|
@mhegazy I will create a PR for that shortly. |
|
thanks! |
| @@ -237,6 +237,8 @@ namespace ts.codefix { | |||
| return parent as ImportDeclaration; | |||
| case SyntaxKind.ExternalModuleReference: | |||
| return (parent as ExternalModuleReference).parent; | |||
| case SyntaxKind.ImportEqualsDeclaration: | |||
There was a problem hiding this comment.
This will never be the parent of a LiteralExpression. Removing in #19667.
As described in #17860, the import code fix for UMD modules has been updated such that it
import m from "specifier";import under --allowSyntheticDefaultImportsimport m = require("specifier");syntax where supportedimport * as m from "specifier";syntax where necessaryFixes #17860
I'm not sure how to go about writing unit tests for this.