-
Notifications
You must be signed in to change notification settings - Fork 559
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
Support named exports in Node.js native ES2015 modules implementation #38
base: main
Are you sure you want to change the base?
Conversation
|
||
- ESM code **must** be inside of a `.mjs` file | ||
- ESM code **must** use ES2015 `import` and `export` and cannot use `require` | ||
- ESM code **must** declare all of its named exports in the `.mjs` file or they must come from another `.mjs` file exported using `export * from './somefile'`. (You likely could use `*` on a CommonJS module, but it would only export `default`) |
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.
export * from
explicitly ignores reexporting a default
export; so it'd export nothing.
To completely reexport all exports of a module you need both export * from
and export { default } from
for the same module.
export const cloneElement = React.cloneElement; | ||
export const createFactory = React.createFactory; | ||
export const isValidElement = React.isValidElement; | ||
export const version = React.version; |
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 can even be drastically simplified to:
export const {
Children,
createRef,
Component,
// ... etc
} = React
- Alternatively, React.js could exclusively export named exports and an alternative to `index.js` could both `export * from './React';` and `import * as React from 'react'; export default React;` to get the `React` object as a free side effect. | ||
- The build process will need to output `./esm/react.{production.min,development}.mjs` bundles in addition to the normal `./cjs/` bundles. I expect these will just be the Rollup bundle already being generated but without the module transpilation step. | ||
- A `index.mjs` will sit at the root beside `index.js`, it will act the same as `index.js` but will export the `./esm` bundles using ES2015 exports instead of the `./cjs` bundles. | ||
- Unresolved: It is not actually possible to do the `if (process.env.NODE_ENV === 'production') {` conditional that `index.js` does in ESM, ES modules require that import is at the top of the file and does not permit conditional importing. |
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.
It is possible but only with dynamic import()
, and that also requires that anything that must go through that code path be made async.
It'd still work without going with async imports if the module body itself has no side-effects; even if imported, it'll just do nothing, and can be removed by dead code elimination.
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.
I think the most natural solution is to do conditional re-exports.
Like
export const createElement = (process.env.NODE_ENV === 'production') ? createElementProd : createElementDev;
in the entry point. I don't think the ecosystem is ready for this yet though, given how fragile tree shaking is.
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.
Here's how you can test treeshakability without assumptions.
https://github.com/TrySound/rollup-plugin-size-snapshot/blob/master/src/treeshakeWithWebpack.js#L15
|
||
Alternatively we could decide that we are not going to support named imports of React from ESM. | ||
|
||
This will not be a breaking change. Use of .mjs and ESM is opt-in functionality. All packages using babel or a bundler to access node will keep working with the non-spec compliant handling of named exports from CommonJS modules. |
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.
Unless their build system automatically chooses mjs when it's available. In this case it would be breaking.
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.
.mjs files aren't supposed to be imported from require in .js files (including all current babel ES6 import usage in .js, which really uses require behaviour under the hood).
In fact the esm page explicitly states that "require('./foo.mjs')
is unsupported because "ES Modules have differing resolution and timing".
Any build-system automatically choosing .mjs when available would be violating ESM.
|
||
This will not be a breaking change. Use of .mjs and ESM is opt-in functionality. All packages using babel or a bundler to access node will keep working with the non-spec compliant handling of named exports from CommonJS modules. | ||
|
||
The only requirement will be that if someone decides to switch to `.mjs` and opt-in to using ESM instead of transpiling/bundling, they will be required to refactor their entire codebase to use `React.Component` instead of named exports. |
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.
Also that 90% of tutorials / examples will be wrong. Seems like a pretty bad situation to be in. On the other hand TS already enforces using named imports only. I dunno.
It seems like it's reasonable to provide a default export but slowly encourage people to using named or namespace imports instead? And maybe eventually drop the default one.
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.
I already migrated in a bunch of react packages. I think the best thing we can do is a migration to import * React from 'react'
in docs and blaming import React, { Component } from 'react'
which doesn't fit to any way. I could start this migration if you will write clean post in this thread.
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.
You can use default exports with TypeScript. But it is true that using default import for React from TypeScript does not work.
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.
Migrating to import * as React from 'react'
is the wrong migration to do; it only "works" if you are using TypeScript's old commonjs emit. You should be using --esModuleInterop
and using import React from 'react'
instead.
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.
Can't typescript handle both cases with interop like everybody does? Rollup, webpack, babel handle this without problems.
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.
It "can", and you need to give --esModuleInterop
. However, my point is that if you use --module es2015
or --module esnext
, the resulting output (if renamed to .mjs
as it needs to be) will be broken and crash at runtime on node unless you use the default
import.
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.
Can this be fixed in perspective so we could start migrating to import * as React from 'react'
. Or do you think we stuck with default export forever?
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.
That is what this RFC is meant to address.
It is unknown when a node will support ESM and what will be the form. https://github.com/nodejs/modules Modules team in node did not publish timeline and previous PRs in the node were postponed. This is already the third attempt of ESM in the node. IMO focus should be on browser ESM implementation that works now. |
We also need to consider what to do with |
We can do the same or just change exports to import { TestRenderer, Shallow } from 'react-test-renderer'; |
I think the general thinking here is that we want to do something like this but it'd be a breaking change and only worth doing along with other breaking changes...? |
The Node.js ESM implementation has been pushed back to October and has changed a bit in implementation. So the RFC is kind of on hold. Theoretically this change should not be breaking since it only affects people writing It's tangentially related. But I wonder if we should also revisit TypeScript. It's really annoying having to use |
@dantman Default export is not the right way to import from react. React.createElement()
// to
const { createElement } = React
createElement() When bundler may do this for free and even better will not generate Treeshaking will not be significant with react but it still can be considered as a feature. |
React should be a namespace. It currently is not. |
You're going to have to rally for JSX to be changed then. Because the default is
Treeshaking will be completely nonexistent. React is bundled into a single file and uses an object export. |
Not at all.
I'm talking about the time when react will have esm and named exports. |
React's single file bundles were an intentional choice, even if React ever gets ESM it will likely still use single file bundles. |
@dantman Wait, I didn't say anything about react bundles. I like them. My point was that react should not have default export because it increases user bundle a lot or requires babel plugins to solve the problem and still with trade offs. |
How does this proposal hold up against the latest shipped Node implementation? |
There seem to be some changes to the ESM handling. The general idea of the RFC seems to still be valid, but about 10% needs a few tweaks for the ESM changes. Scanning the new https://nodejs.org/api/esm.html docs here are the things that stand out:
@gaearon Is there any desire to implement this anytime soon? If so I can revisit this now and update the RFC. Otherwise I can revisit it the next time there is new news on Node's ESM implementation (changes from experimental to stable, finishes the loader API, other toolkits adopt the ESM behaviours, or a number of React users indicate they wish to use ESM). |
The import specifiers in ESM E.g. when importing ESM into ESM: - import { Foo } from './Foo'
+ import { Foo } from './Foo.mjs' E.g. when importing CJS into ESM: - import Foo from './Foo'
+ import Foo from './Foo.js' |
"Soon" is relative but it's always helpful to have an up-to-date plan in case it seems like the right moment to start the work. In particular, I'm worry about how to be compatible with Node, bundlers, Flow/TS, and everything else. |
|
||
Here are the requirements that the ESM implementation in Node.js place on us and it's behaviours we can take advantage of: | ||
|
||
- ESM code **must** be inside of a `.mjs` file |
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 technically correct - you can use .js
if you put within a directory containing package.json with {"type": "module"}
and you can even use .js
extension for both CJS and ESM entries, u "just" need to extra package.json files. For example, Rollup is using this "trick": https://unpkg.com/browse/rollup@2.23.0/dist/es/
This could have not been true at the time of writing this RFC though.
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 could have not been true at the time of writing this RFC though.
Correct. At the time of writing the RFC this is how the Node.js ESM implementation worked. It was only available in .mjs files and the type
in package.json did not exist. The implementation has changed significantly and type
was noted in my list of differences.
For React's purpose though, .mjs
would still be used for the ES module wrapper unless the project were to make a large breaking change switching entirely to a ES modules codebase.
This RFC seems to be getting some attention at the moment. I’d like to direct any folks who are interested in it to the Node.js documentation, and in particular the “Dual CommonJS/ES module packages” section which provides a more complete and up-to-date picture: https://nodejs.org/dist/latest-v14.x/docs/api/packages.html#packages_dual_commonjs_es_module_packages There is also an ongoing discussion in facebook/react#11503. |
Our latest thinking was that we'd drop default exports altogether. Named only. |
Perhaps this should be deprecated starting React 19? |
View formatted RFC
I would appreciate it if we kept the merits of
.mjs
and how Node.js chose to implement ES2015 modules out of the discussion here. We should just accept the fact that ES2015 modules are getting a native implementation in Node.js, this is what we have to work with, and focus on fixing React so that it works with the implementation we are getting.