FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix inconsistencies in import UMD code fixes adapting to module format by aluanhaddad · Pull Request #19572 · microsoft/TypeScript · GitHub

/ TypeScript Public

fix inconsistencies in import UMD code fixes adapting to module format - #19572

Merged
Mohamed Hegazy (mhegazy) merged 6 commits into
microsoft:masterfrom
aluanhaddad:module-format-aware-import-fixes
Nov 7, 2017
ConversationCommits (6)ChecksFiles changed
Merged

fix inconsistencies in import UMD code fixes adapting to module format#19572
Mohamed Hegazy (mhegazy) merged 6 commits into
microsoft:masterfrom
aluanhaddad:module-format-aware-import-fixes

Conversation

Aluan Haddad (aluanhaddad) commented Oct 30, 2017

Copy link
Copy Markdown
Contributor

As described in #17860, the import code fix for UMD modules has been updated such that it

  • uses import m from "specifier"; import under --allowSyntheticDefaultImports
  • uses import m = require("specifier"); syntax where supported
  • falls back to import * as m from "specifier"; syntax where necessary
  • displays a diagnostic message matching the import that results from applying it

Fixes #17860

I'm not sure how to go about writing unit tests for this.

Mohamed Hegazy (mhegazy) suggested changes Oct 30, 2017
View reviewed changes
Comment thread src/services/codefixes/importFixes.ts Outdated
const { module, allowSyntheticDefaultImports } = context.compilerOptions;

// Prefer to import as a synthetic `default` if available.
if (allowSyntheticDefaultImports || module === ModuleKind.System && allowSyntheticDefaultImports !== false) {

Mohamed Hegazy (mhegazy) Oct 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Add a function to src\core.ts to getAllowSyntheticDefaultImports(compilerOptions) that has this logic, and share it with the checker as well.

Comment thread src/services/codefixes/importFixes.ts Outdated
Debug.fail("Either the symbol or the JSX namespace should be a UMD global if we got here");
}

const { module, allowSyntheticDefaultImports } = context.compilerOptions;

Mohamed Hegazy (mhegazy) Oct 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

consider moving compilerOptions to top destructuring declaration.

Comment thread src/services/codefixes/importFixes.ts Outdated
}

// 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) {

Mohamed Hegazy (mhegazy) Oct 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

not sure i agree we need this change.

Aluan Haddad (aluanhaddad) Nov 1, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Aluan Haddad (aluanhaddad) Nov 2, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Mohamed Hegazy (mhegazy) commented Oct 30, 2017

Copy link
Copy Markdown
Contributor

For tests, take a look at tests/cases/fourslash/importNameCodeFixNewImportFileDetachedComments.ts

Aluan Haddad (aluanhaddad) force-pushed the module-format-aware-import-fixes branch from 266ea5f to af6b43a Compare November 1, 2017 16:52
improve import code fixes for UMD modules
dcbadcb 
 - use default import under --allowSyntheticDefaultImports
 - import..require support
 - make make quick fix info match resulting import
 - make diagnostics
Address PR feedback:
3041e10 
 - 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
add tests
1be8b21
Aluan Haddad (aluanhaddad) force-pushed the module-format-aware-import-fixes branch from 06cd3c3 to 1be8b21 Compare November 4, 2017 20:30
Mohamed Hegazy (mhegazy) reviewed Nov 6, 2017
View reviewed changes
Comment thread src/services/codefixes/importFixes.ts
const allowSyntheticDefaultImports = getAllowSyntheticDefaultImports(compilerOptions);

// Prefer to import as a synthetic `default` if available.
if (allowSyntheticDefaultImports) {

Mohamed Hegazy (mhegazy) Nov 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Wesley Wigham (@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

Aluan Haddad (aluanhaddad) Nov 6, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Roger. Thank you!

Mohamed Hegazy (mhegazy) commented Nov 6, 2017

Copy link
Copy Markdown
Contributor

Aluan Haddad (@aluanhaddad) can you please address the test failures.

Aluan Haddad (aluanhaddad) commented Nov 7, 2017

Copy link
Copy Markdown
Contributor Author

I updated it to always uses the synthetic defaults for UMD modules unless --getAllowSyntheticDefaultImports(compilerOptions) returns false in which case it suggests a namespace import. This also resolves the test failures.

Mohamed Hegazy (mhegazy) merged commit 70cabdd into microsoft:master Nov 7, 2017

Mohamed Hegazy (mhegazy) commented Nov 7, 2017

Copy link
Copy Markdown
Contributor

thanks Aluan Haddad (@aluanhaddad)!

Want to redo the import .. =require support for AMD/CommonJS/UMD? I think we should do that as well.

Aluan Haddad (aluanhaddad) commented Nov 7, 2017

Copy link
Copy Markdown
Contributor Author

Mohamed Hegazy (@mhegazy) I will create a PR for that shortly.

Mohamed Hegazy (mhegazy) commented Nov 7, 2017

Copy link
Copy Markdown
Contributor

thanks!

Aluan Haddad (aluanhaddad) deleted the module-format-aware-import-fixes branch November 7, 2017 08:33
Aluan Haddad (aluanhaddad) mentioned this pull request Nov 7, 2017
Merged
Deleted user (ghost) reviewed Nov 7, 2017
View reviewed changes
Comment thread src/services/codefixes/importFixes.ts
return parent as ImportDeclaration;
case SyntaxKind.ExternalModuleReference:
return (parent as ExternalModuleReference).parent;
case SyntaxKind.ImportEqualsDeclaration:

Deleted user (ghost) Nov 7, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This will never be the parent of a LiteralExpression. Removing in #19667.

Microsoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
Mohamed Hegazy (mhegazy) mhegazy requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Quick Fix info inconsistent when offering to import a UMD declaration.

2 participants

Back | FazBrowse Home | New Git URL