Skip to content
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

Add module: hybrid #29353

Closed

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jan 10, 2019

Oftentimes TS is used for typechecking and then piped into webpack, babel, @std/esm, or the like. The ES6 module format usually works in these cases, however there are sometimes some idiosyncrasies that necessitate usage of some features that aren't strictly es6 module features - for example, importing a callable cjs module via require (rather than thru an import statement) or assigning a (potentially callable) export object yourself. We allow both of these thru import= and export= statements when targeting cjs, and most of these targets (webpack, babel, @std/esm) support mixing these cjs idioms into an es module to one degree or another.

So, in this PR I have modified the es2015 module transformer to, rather than elide import= and export= statements, to emit them as their cjs counterparts (since this is an error, the emit change shouldn't matter). I then add a module: hybrid mode which suppresses the errors we would normally emit on those export= or import= statements, but otherwise behaves like the es2015 module target. This new hybrid target is a pretty optimal middleground for people using hybrid pipelines where the ultimate target is usually cjs-like, but the output of the ts compiler is desired to be es2015-y (or at least have no TS-specific syntax) for code-splitting purposes.

Ref #22321, #22851

cc @bterlson @DanielRosenwasser

@Jessidhia
Copy link

Jessidhia commented Jan 11, 2019

I actually didn't expect webpack to still allow access to require even in modules that use the javascript/esm factory (by default applied to .mjs files). It does ban access to the module object, though, except for its fictitious module.hot API, and still allows access to __dirname and __filename.

Note, though, that webpack does completely ban access to exports and module.exports in any javascript/auto (unambiguous) or javascript/esm module that is detected as an ES module. ES export statements are the single source of truth there, export = would cause a build error farther down the line if any ES imports remain.

I also suspect that this hybrid mode won't be that useful for bundler users without also whitelisting access to the import() statement.


Relevant webpack source code

HarmonyCompatibilityDependency is the __esModule define (added conditionally if webpack can't prove the module's used exports), HarmonyInitDependency is where hoisted imports and exports go; the renaming of exportsArgument is what prevents access to the exports function wrapper value, and the renaming of moduleArgument prevents access to module.

@weswigham
Copy link
Member Author

ES export statements are the single source of truth there, export = would cause a build error farther down the line if any ES imports remain.

The irony there is that if you only use import= and export= in the file it'll look exactly like a cjs file for webpack once compiled by TS under this scheme.

@Jessidhia
Copy link

@weswigham yep, that's why I clarified that is "if any ES imports remain" 😄

@Jessidhia
Copy link

Also, looking at the CommonJsPlugin code, it seems the require parser is only installed if the module is not javascript/esm; so unambiguous (javascript/auto) modules do still get to require even if they're detected as modules.

@Jessidhia
Copy link

(Personally I think it's a bad idea to use this hybrid mode, you instantly prevent any module from being tree-shaked, or at least get their export names minified, the moment you require() them, but I'm not against the feature itself; looks useful especially to people who are still migrating)

Also, a question: would such a module that only has import= and export= be parsed/emitted in strict mode or sloppy mode?

@DanielRosenwasser
Copy link
Member

Personally I think it's a bad idea to use this hybrid mode, you instantly prevent any module from being tree-shaked

I don't think see why a module loader would be any more capable of tree-shaking a default import over a require'd module if the source is CommonJS.

In general I'm not big on the name "hybrid" because it's so vague as to what it's a hybrid of (i.e. CommonJS/ESM). It's also not 100% clear whether this hybrid mode is going to be compatible with whatever Node.js supports either, which concerns me.

I also feel like the user story will now be even more confusing giving users the options between module targets:

  • commonjs
  • esnext
  • hybrid

and emit/check options:

  • allowSyntheticDefaultImports
  • esModuleInterop

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

I don't see any checks to set the default option for moduleResolution - this new mode should set it to Node.

@weswigham
Copy link
Member Author

@DanielRosenwasser Thanks for the reminder, done.

@DanielRosenwasser DanielRosenwasser added this to 𝚜𝚘𝚘𝚗 ™ in Design Meeting Docket Jan 25, 2019
@RyanCavanaugh RyanCavanaugh moved this from 𝚜𝚘𝚘𝚗 ™ to Next Week in Design Meeting Docket Feb 1, 2019
@RyanCavanaugh RyanCavanaugh assigned weswigham and unassigned weswigham Apr 30, 2019
@DanielRosenwasser DanielRosenwasser moved this from Next Week to 𝚜𝚘𝚘𝚗 ™ in Design Meeting Docket May 24, 2019
@justinfagnani
Copy link

I don't see any checks to set the default option for moduleResolution - this new mode should set it to Node.

If the goal is to be compatible with Node.js supports (another reason why "hybrid" might not be a good name) then this type of file will actually need two resolutions modes: node for require() and something new for imports (Node isn't doing the path searching on imports, extensions will be required, etc.)

@sandersn sandersn added this to Bête Noire in Pall Mall Jan 29, 2020
@sandersn sandersn added the Experiment A fork with an experimental idea which might not make it into master label Feb 1, 2020
@jkrems
Copy link

jkrems commented Mar 13, 2020

Drive-by comment from the future.

If the goal is to be compatible with Node.js supports (another reason why "hybrid" might not be a good name)

I think it's meant to be compatible with bundlers, not with node. Node doesn't allow using CommonJS APIs in modules. But those bundlers happen to use the node (or rather CommonJS) resolution algorithm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Experiment A fork with an experimental idea which might not make it into master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants