Skip to content

Convert react to named exports#13080

Closed
TrySound wants to merge 2 commits into
react:masterfrom
TrySound:react-named-exports
Closed

Convert react to named exports#13080
TrySound wants to merge 2 commits into
react:masterfrom
TrySound:react-named-exports

Conversation

@TrySound

@TrySound TrySound commented Jun 20, 2018

Copy link
Copy Markdown
Contributor

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.

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

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

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;

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;
```
@gaearon

gaearon commented Jun 20, 2018

Copy link
Copy Markdown
Collaborator

Wouldn't we want to support import React from 'react' in an ESM environment too? While not ideal, not doing this could be pretty bad for all the tutorials and examples out there.

@TrySound

TrySound commented Jun 20, 2018

Copy link
Copy Markdown
Contributor Author

We will do this in index.mjs with NODE_ENV conditions. There's no need to duplicate work in two files.

@gaearon

gaearon commented Jun 20, 2018

Copy link
Copy Markdown
Collaborator

If this is a part of a larger plan, can you describe the whole plan?

@TrySound

Copy link
Copy Markdown
Contributor Author

Now we provide only cjs with object of named exports. Adding default export in React.js will add new default field in that object. This is another one way to import module which we should omit.

The plan is to add index.mjs which will import from esm bundles and reexport fields with NODE_ENV condition like this

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.

@TrySound

Copy link
Copy Markdown
Contributor Author

How to deal with types? It's not clear for me how this repo configured flow.

@gaearon

gaearon commented Jun 20, 2018

Copy link
Copy Markdown
Collaborator

Adding default export in React.js will add new default field in that object.

So can we do it in this PR? Is something preventing this?

@TrySound

Copy link
Copy Markdown
Contributor Author

Do you want this to keep all internal imports unchanged?

@gaearon

gaearon commented Jun 20, 2018

Copy link
Copy Markdown
Collaborator

No, I just think it's confusing that we want to support a default export as public API, but then the React entry point looks like it doesn't have a default export.

@TrySound

Copy link
Copy Markdown
Contributor Author

Okay. This allows us to not change build configuration. I checked bundles with babel-jest, webpack and rollup. Everything works as expected.

Comment thread packages/react/index.js Outdated
*/

export * from './src/React';
export {default} from './src/React';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Isn't export * going to be sufficient to cover default too?

@TrySound TrySound Jun 20, 2018

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Haha okay, TIL.

Comment thread packages/react/src/React.js Outdated

export const Timeout = enableSuspense ? REACT_TIMEOUT_TYPE : null;

export default {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's name it, const React = { ... } and then export default React.

@pull-bot

pull-bot commented Jun 20, 2018

Copy link
Copy Markdown

React: size: 🔺+6.5%, gzip: 🔺+5.9%

Details of bundled changes.

Comparing: 97af3e1...c687a96

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +1.5% +0.7% 56.81 KB 57.64 KB 15.82 KB 15.93 KB UMD_DEV
react.production.min.js 🔺+6.5% 🔺+5.9% 6.86 KB 7.3 KB 2.91 KB 3.08 KB UMD_PROD
react.development.js +1.6% +0.7% 51 KB 51.81 KB 13.99 KB 14.09 KB NODE_DEV
react.production.min.js 🔺+9.0% 🔺+6.0% 5.86 KB 6.39 KB 2.52 KB 2.67 KB NODE_PROD
React-dev.js +1.6% +0.8% 48.63 KB 49.43 KB 13.28 KB 13.38 KB FB_WWW_DEV
React-prod.js 🔺+6.4% 🔺+3.5% 13.38 KB 14.24 KB 3.73 KB 3.86 KB FB_WWW_PROD

Generated by 🚫 dangerJS

@TrySound

Copy link
Copy Markdown
Contributor Author

The size became slightly bigger because of name duplicating.

@gaearon

gaearon commented Jun 20, 2018

Copy link
Copy Markdown
Collaborator

What's the difference in prettified production output? The size difference is unexpected to me.

@TrySound

Copy link
Copy Markdown
Contributor Author
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});

@gaearon

gaearon commented Jun 20, 2018

Copy link
Copy Markdown
Collaborator

Okay. Is the size better if we leave React.js as named exports only, and then do

import * as React from './src/React';
export * from './src/React';
export {React as default};

?

@TrySound

Copy link
Copy Markdown
Contributor Author

This will freeze exported object.

@TrySound

Copy link
Copy Markdown
Contributor Author

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.

@TrySound

Copy link
Copy Markdown
Contributor Author

And we don't need to disable freezing with it.

@gaearon

gaearon commented Jun 20, 2018

Copy link
Copy Markdown
Collaborator

Why not disable freezing?

Seems like as long as we want to support monkeypatching we'd always have to disable it anyway.

@TrySound

TrySound commented Jun 20, 2018

Copy link
Copy Markdown
Contributor Author

For some reason it's enabled for development builds.

@TrySound

Copy link
Copy Markdown
Contributor Author

#12879

@gaearon

gaearon commented Jun 20, 2018

Copy link
Copy Markdown
Collaborator

We could disable it completely. It was only enabled in the first place because it was Rollup default.

@TrySound

Copy link
Copy Markdown
Contributor Author

Sure. Will disable then.

@TrySound

Copy link
Copy Markdown
Contributor Author

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;

@TrySound

Copy link
Copy Markdown
Contributor Author

So should we go with my original solution?

@gaearon

gaearon commented Jun 20, 2018

Copy link
Copy Markdown
Collaborator

Okay. I'll still need to review that another time, but at least we shouldn't increase the size.

@TrySound

Copy link
Copy Markdown
Contributor Author

Updated PR. Here's what we have

master 5.86kb
export * from 6.00kb (current solution)
export * and default 6.38kb

0.14kb is duplicated exports keyword

count,
toArray,
only,
};

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.

Why not import * as Children from './ReactChildren'; export { Children };? If ReactChildren exports only the public API, this would allow for tree shaking.

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.

Afaik rollup is able to treeshake object literals. Not sure webpack able to handle any of cases.

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.

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.

@Jessidhia Jessidhia Oct 29, 2018

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.

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).

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.

@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;

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.

: undefined

Comment thread scripts/rollup/build.js
},
},
// remove Object.defineProperty(exports, '__esModule', { value: true })
// to allow both import React from 'react' and import * as React from 'react'

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.

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 }.

@TrySound TrySound Jun 30, 2018

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.

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.

@stale

stale Bot commented Jan 10, 2020

Copy link
Copy Markdown

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.

@stale stale Bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@stale

stale Bot commented Jan 17, 2020

Copy link
Copy Markdown

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!

@stale stale Bot closed this Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Resolution: Stale Automatically closed due to inactivity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants