Convert react to named exports#13080
Conversation
In this diff I'm making a step forward to esm support. Here I export scoped variables instead of object. Conditions are resolved with with ternary operator. To solve the problem of using both `import * as React from 'react'` and `import React from 'react'` I removed `__esModule` field with simple regexp. In the future rollup release this will be achived with [new rollup option](rollup/rollup#2287). New bundles are tested with webpack, rollup and babel-jest. All ways produce same results. There was a problem with reexports in #11526 and as you can see in examples below it's solved so we can migrate react-dom to named exports aswell in the next PR. before ```js var React = { Children: { map: mapChildren, forEach: forEachChildren, count: countChildren, toArray: toArray, only: onlyChild }, createRef: createRef, Component: Component, PureComponent: PureComponent, createContext: createContext, forwardRef: forwardRef, Fragment: REACT_FRAGMENT_TYPE, StrictMode: REACT_STRICT_MODE_TYPE, unstable_AsyncMode: REACT_ASYNC_MODE_TYPE, unstable_Profiler: REACT_PROFILER_TYPE, createElement: createElementWithValidation, cloneElement: cloneElementWithValidation, createFactory: createFactoryWithValidation, isValidElement: isValidElement, version: ReactVersion, __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: { ReactCurrentOwner: ReactCurrentOwner, // Used by renderers to avoid bundling object-assign twice in UMD bundles: assign: _assign } }; if (enableSuspense) { React.Timeout = REACT_TIMEOUT_TYPE; } { _assign(React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, { // These should not be included in production. ReactDebugCurrentFrame: ReactDebugCurrentFrame, // Shim for React DOM 16.0.0 which still destructured (but not used) this. // TODO: remove in React 17.0. ReactComponentTreeHook: {} }); } var React$2 = Object.freeze({ default: React }); var React$3 = ( React$2 && React ) || React$2; // TODO: decide on the top-level export form. // This is hacky but makes it work with both Rollup and Jest. var react = React$3.default ? React$3.default : React$3; module.exports = react; ``` after ```js exports.Children = Children; exports.createRef = createRef; exports.Component = Component; exports.PureComponent = PureComponent; exports.createContext = createContext; exports.forwardRef = forwardRef; exports.isValidElement = isValidElement; exports.Fragment = Fragment; exports.StrictMode = StrictMode; exports.unstable_AsyncMode = unstable_AsyncMode; exports.unstable_Profiler = unstable_Profiler; exports.createElement = createElement$$1; exports.cloneElement = cloneElement$$1; exports.createFactory = createFactory$$1; exports.version = version; exports.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED; exports.Timeout = Timeout; ```
|
Wouldn't we want to support |
|
We will do this in |
|
If this is a part of a larger plan, can you describe the whole plan? |
|
Now we provide only cjs with object of named exports. Adding default export in The plan is to add import * as dev from './esm/react.development.mjs';
import * as prod from './esm/react.production.min.mjs';
export const createElement =
process.env.NODE_ENV === 'production'
? prod.createElement
: dev.createElement;In this PR I'm just trying to achieve cleaner exports. |
|
How to deal with types? It's not clear for me how this repo configured flow. |
So can we do it in this PR? Is something preventing this? |
|
Do you want this to keep all internal imports unchanged? |
|
No, I just think it's confusing that we want to support a default export as public API, but then the |
|
Okay. This allows us to not change build configuration. I checked bundles with babel-jest, webpack and rollup. Everything works as expected. |
| */ | ||
|
|
||
| export * from './src/React'; | ||
| export {default} from './src/React'; |
There was a problem hiding this comment.
Isn't export * going to be sufficient to cover default too?
There was a problem hiding this comment.
Nope. That was a surprise to me too.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export
|
|
||
| export const Timeout = enableSuspense ? REACT_TIMEOUT_TYPE : null; | ||
|
|
||
| export default { |
There was a problem hiding this comment.
Let's name it, const React = { ... } and then export default React.
|
React: size: 🔺+6.5%, gzip: 🔺+5.9% Details of bundled changes.Comparing: 97af3e1...c687a96 react
Generated by 🚫 dangerJS |
|
The size became slightly bigger because of name duplicating. |
|
What's the difference in prettified production output? The size difference is unexpected to me. |
var X = {
Children: A,
createRef: B,
Component: C,
PureComponent: D,
createContext: E,
forwardRef: F,
Fragment: G,
StrictMode: H,
unstable_AsyncMode: I,
unstable_Profiler: J,
createElement: K,
cloneElement: L,
createFactory: M,
isValidElement: N,
version: O,
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: P,
},
Y = {default: X},
Z = (Y && X) || Y;
module.exports = Z.default ? Z.default : Z; d['default'] = {
Children: h,
createRef: G,
Component: l,
PureComponent: t,
createContext: N,
forwardRef: O,
isValidElement: u,
Fragment: Q,
StrictMode: R,
unstable_AsyncMode: T,
unstable_Profiler: S,
createElement: w,
cloneElement: U,
createFactory: V,
version: '16.4.1',
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: W,
Timeout: null,
};
d.Children = h;
d.createRef = G;
d.Component = l;
d.PureComponent = t;
d.createContext = N;
d.forwardRef = O;
d.isValidElement = u;
d.Fragment = Q;
d.StrictMode = R;
d.unstable_AsyncMode = T;
d.unstable_Profiler = S;
d.createElement = w;
d.cloneElement = U;
d.createFactory = V;
d.version = '16.4.1';
d.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = W;
d.Timeout = null;
Object.defineProperty(d, '__esModule', {value: !0}); |
|
Okay. Is the size better if we leave import * as React from './src/React';
export * from './src/React';
export {React as default};? |
|
This will freeze exported object. |
|
The size was better with my previous solution. The only downside of it is missing default export in development and production esm bundles. It will exist only in index.mjs. |
|
And we don't need to disable freezing with it. |
|
Why not disable freezing? Seems like as long as we want to support monkeypatching we'd always have to disable it anyway. |
|
For some reason it's enabled for development builds. |
|
We could disable it completely. It was only enabled in the first place because it was Rollup default. |
|
Sure. Will disable then. |
|
Here's the new result. The same size problem. exports['default'] = {
Children: Y,
createRef: G,
Component: C,
PureComponent: E,
createContext: W,
forwardRef: X,
isValidElement: N,
Fragment: r,
StrictMode: t,
unstable_AsyncMode: x,
unstable_Profiler: u,
createElement: K,
cloneElement: M,
createFactory: L,
version: '16.4.1',
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: Z,
Timeout: null,
};
exports.Children = Y;
exports.createRef = G;
exports.Component = C;
exports.PureComponent = E;
exports.createContext = W;
exports.forwardRef = X;
exports.isValidElement = N;
exports.Fragment = r;
exports.StrictMode = t;
exports.unstable_AsyncMode = x;
exports.unstable_Profiler = u;
exports.createElement = K;
exports.cloneElement = M;
exports.createFactory = L;
exports.version = '16.4.1';
exports.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = Z;
exports.Timeout = null; |
|
So should we go with my original solution? |
|
Okay. I'll still need to review that another time, but at least we shouldn't increase the size. |
|
Updated PR. Here's what we have master 5.86kb 0.14kb is duplicated |
| count, | ||
| toArray, | ||
| only, | ||
| }; |
There was a problem hiding this comment.
Why not import * as Children from './ReactChildren'; export { Children };? If ReactChildren exports only the public API, this would allow for tree shaking.
There was a problem hiding this comment.
Afaik rollup is able to treeshake object literals. Not sure webpack able to handle any of cases.
There was a problem hiding this comment.
Why not import * as Children from './ReactChildren'; export { Children };? If ReactChildren exports only the public API, this would allow for tree shaking.
Would it? I would expect Children becoming an object after such a thing and thus preventing tree-shaking for less sophisticated algorithms. I don't think namespace reexports are tree-shakeable by webpack, although I might be wrong.
There was a problem hiding this comment.
Only namespace reexports are tree-shakeable by webpack; by storing the result in an actual object you prevent webpack's tree shaking.
Even if it somehow does work anyway, you're still creating an unnecessary additional object that doesn't follow the rule of namespaces (no null prototype for example).
There was a problem hiding this comment.
@Kovensky not sure if you statements agrees with me or not, just to test this once again - even if only for myself - I've prepared a quick test to see if they are treeshakeable by webpack or not and according to my results they are not
https://github.com/Andarist/webpack-reexported-namespace-test/blob/cf141855cf341a9cf0ea673d760546f77438e6ca/dist/main.js#L47-L54
| } | ||
|
|
||
| export default React; | ||
| export const Timeout = enableSuspense ? REACT_TIMEOUT_TYPE : null; |
| }, | ||
| }, | ||
| // remove Object.defineProperty(exports, '__esModule', { value: true }) | ||
| // to allow both import React from 'react' and import * as React from 'react' |
There was a problem hiding this comment.
This does not allow both unless you're using babelmodules; please don't write things for babelmodules other than maybe in a compatibility bundle. Write things to work on real modules.
Real ES modules will only be able to import * as React from from the non-transpiled version, and only be able to import React from from the transpiled version. This is why it's important to have a .mjs entry point, so that every place is able to do import * as React from correctly.
If you want to still preserve import React from even in the .mjs entry point you're going to need an export default { ...list of all the exports }.
There was a problem hiding this comment.
Reexport with default will be in mjs entry point with NODE_ENV conditions. This is only bundle entry point. This hack is only for cjs.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
In this diff I'm making a step forward to esm support.
Here I export scoped variables instead of object. Conditions are
resolved with with ternary operator.
To solve the problem of using both
import * as React from 'react'and
import React from 'react'I removed__esModulefield with simpleregexp. In the future rollup release this will be achived with
new rollup option.
New bundles are tested with webpack, rollup and babel-jest. All ways
produce same results.
There was a problem with reexports in #11526
and as you can see in examples below it's solved so we can migrate
react-dom to named exports aswell in the next PR.
before
after