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

Formalize top-level ES exports #11503

Open
gaearon opened this issue Nov 9, 2017 · 55 comments
Open

Formalize top-level ES exports #11503

gaearon opened this issue Nov 9, 2017 · 55 comments

Comments

@gaearon
Copy link
Member

@gaearon gaearon commented Nov 9, 2017

Currently we only ship CommonJS versions of all packages. However we might want to ship them as ESM in the future (#10021).

We can't quite easily do this because we haven't really decided on what top-level ES exports would look like from each package. For example, does react have a bunch of named exports, but also a default export called React? Should we encourage people to import * for better tree shaking? What about react-test-renderer/shallow that currently exports a class (and thus would start failing in Node were it converted to be a default export)?

@Andarist

This comment has been minimized.

Copy link
Contributor

@Andarist Andarist commented Nov 9, 2017

Imho import * is a way to go, Im not opposed to having a default export too, but it shouldnt be used to reexport other stuff like in this example:

export const Component = ...
export default React
React.Component = Component
@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Nov 9, 2017

but it shouldnt be used to reexport other stuff like in this example:

Is there a technical reason why? (Aside from having two ways to do the same thing.)

My impression is that people who would import * (and not use the default) wouldn't have problems tree shaking since default would stay unused. But maybe I overestimate Rollup etc.

@Andarist

This comment has been minimized.

Copy link
Contributor

@Andarist Andarist commented Nov 9, 2017

That questions can be probably best answered by @lukastaegert. Ain't sure if something has changed since #10021 (comment)

Also Rollup is not the only tree shaker out there, and while webpack's tree-shaking algorithm is worse than the one in rollup, it's usage is probably way higher than rollup's (both tools do excellent jobs ofc, I don't want to offend anyone, just stating facts) and if we can (as the community) help both tools at once we should do so whenever we can.

@jquense

This comment has been minimized.

Copy link
Collaborator

@jquense jquense commented Nov 9, 2017

is tree-shaking going to do anything in React's case, given that everything is preprocessed into a single flat bundle? I wonder what the primary import style is for React, personally i tend to treat it like a default export e.g. React.Component, React.Children but occasionally do the named thing with cloneElement

@lukastaegert

This comment has been minimized.

Copy link

@lukastaegert lukastaegert commented Nov 10, 2017

As @gaearon already stated elsewhere, size improvements in case of react are expected to be minimal. Nevertheless, there ARE advantages:

  • React.Children might probably be removed in some cases (so I heard 😉)
  • React itself can be hoisted into the top scope by module bundlers that support this. This could again remove quite a few bytes and might also grant an oh-so-slight performance improvement. The main improvement would lie in the fact that there does not need to be another variable that references React.Component for every module but just one that is shared everywhere (this is how rollup usually does it). Also, though this is just me guessing, this might reduce the chance of webpack's ModuleConcatenationPlugin bailing out
  • Static analysis for react is easier not only for module bundlers but also for e.g. IDEs and other tools. Many such tools already do a reasonable job at this for CJS modules but in the end, there is a lot of guessing involved on their side. With ES6 modules, analysis is a no-brainer.

As for the kind of exports, of course only named export really provide the benefit of easy tree-shaking (unless you use GCC which might be able to do a little more in its aggressive move and maybe the latest rollup if you are really lucky). The question if you provide a default export as well is more difficult to decide:

  • PRO: Painless migration for existing ES6 code bases (e.g. what @jquense describes)
  • CON: Since everything is attached to a common object, once this object is included, all its keys are included at once which again defeats any attempts at tree-shaking. Even GCC might have a hard time here.

As a two-version migration strategy, you might add a default export in the next version for compatibility purposes which is declared deprecated (it might even display a warning via a getter etc.) and then remove it in a later version.

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Nov 13, 2017

This is also an interesting case: #11526. While monkeypatching for testing is a bit shady, we'll want to be conscious about breaking this (or having a workaround for it).

@Rich-Harris

This comment has been minimized.

Copy link
Contributor

@Rich-Harris Rich-Harris commented Nov 23, 2017

Came here via this Twitter conversation. For me, there's a clear correct answer to this question: React and ReactDOM should only export named exports. They're not objects that contain state, or that other libraries can mutate or attach properties to (#11526 notwithstanding) — the only reason they exist is as a place to 'put' Component, createElement and so on. In other words, namespaces, which should be imported as such.

(It also makes life easier for bundlers, but that's neither here nor there.)

Of course, that does present a breaking change for people currently using a default import and transpiling. @lukastaegert probably has the right idea here, using accessors to print deprecation warnings. These could be removed in version 17, perhaps?

I don't have a ready-made suggestion for #11526 though. Perhaps shipping ESM would have wait for v17 for that reason anyway, in which case there'd be no need to worry about deprecation warnings.

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Nov 23, 2017

People have really come to like

import React, { Component } from 'react'

so convincing them to give it up might be difficult.

I guess this is not too bad, even if a bit odd:

import * as React from 'react';
import { Component } from 'react';

To clarify, we need React to be in scope (in this case, as a namespace) because JSX transpiles to React.createElement(). We could break JSX and say it depends on global jsx() function instead. Then imports would look like:

import {jsx, Component} from 'react';

which is maybe okay but a huge change. This would also mean React UMD builds now need to set window.jsx too.

Why am I suggesting jsx instead of createElement? Well, createElement is already overloaded (document.createElement) and while it's okay with React. qualifier, without it claiming it on the global is just too much. Tbh I’m not super excited about either of these options, and think this would probably be the best middle ground:

import * as React from 'react';
import { Component } from 'react';

and keep JSX transpiling to React.createElement by default.

@Rich-Harris

This comment has been minimized.

Copy link
Contributor

@Rich-Harris Rich-Harris commented Nov 23, 2017

Confession: I always found it slightly odd that you have to explicitly import React in order to use JSX, even though you're not actually using that identifier anywhere. Perhaps in future, transpilers could insert import * as React from 'react' (configurable for the sake of Preact etc) on encountering JSX, if it doesn't already exist? That way you'd only need to do this...

import { Component } from 'react';

...and the namespace import would be taken care of automatically.

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Nov 23, 2017

In a distant future, maybe. For now we need to make sure transpilers work with other module systems (CommonJS or globals). Making this configurable is also a hurdle, and further splits the community.

@Andarist

This comment has been minimized.

Copy link
Contributor

@Andarist Andarist commented Nov 23, 2017

What @Rich-Harris suggested (inserting a specific import when jsx is used) is easily done by transpilers plugin. The community would have to upgrade their babel-plugin-transform-react-jsx and that's it. And of course even existing setups would still work if only one adds import * as React from 'react'; to the file.

Of course we need to consider other module systems, but it doesn't seem like a hard problem to solve. Are there any specific gotchas in mind?

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Nov 23, 2017

Of course we need to consider other module systems, but it doesn't seem like a hard problem to solve. Are there any specific gotchas in mind?

I don’t know, what is your specific suggestion as to how to handle it? Would what the default be for Babel JSX plugin?

@jamiewinder

This comment has been minimized.

Copy link

@jamiewinder jamiewinder commented Nov 23, 2017

People have really come to like

import React, { Component } from 'react'

What people? Come forth so that I may mock thee.

@gaearon

This comment has been minimized.

Copy link
Member Author

@gaearon gaearon commented Nov 23, 2017

I did that a lot 🙂 Pretty sure I've seen this in other places too.

@Andarist

This comment has been minimized.

Copy link
Contributor

@Andarist Andarist commented Nov 23, 2017

Default is at the moment React.createElement and it would pretty much stay the same. The only problem is that it assumes a global now (or already available in the scope).

I think as es modules are basically the standard way (although not yet adopted by all) of doing modules, it is reasonable to assume majority is (or should) use it. Vast majority already uses various build step tools to create their bundles - which is even more true in this discussion because we are talking about transpiling jsx syntax. Changing the default behaviour of the jsx plugin to auto insertion of React.createElement into the scope is imho reasonable thing to do. We are at the perfect time for this change with babel@7 coming soon (-ish). With recent addition of babel-helper-module-imports it is also easier than ever to insert the right type of the import (es/cjs) to the file.

Having this configurable to bail out to today's behaviour (assuming present in scope) seems really like a minor change in configuration needed for a minority of users and an improvement (sure, not a big one - but still) for majority.

@gaearon gaearon mentioned this issue Nov 28, 2017
1 of 2 tasks complete
@kzc

This comment has been minimized.

Copy link

@kzc kzc commented Dec 3, 2017

Should we encourage people to import * for better tree shaking?

Thanks to @alexlamsl uglify-es has eliminated the export default penalty in common scenarios:

$ cat mod.js 
export default {
	foo: 1,
	bar: 2,
	square: (x) => x * x,
	cube: (x) => x * x * x,
};
$ cat main.js 
import mod from './mod.js'
console.log(mod.foo, mod.cube(mod.bar));
$ rollup main.js -f es --silent | tee bundle.js
var mod = {
	foo: 1,
	bar: 2,
	square: (x) => x * x,
	cube: (x) => x * x * x,
};

console.log(mod.foo, mod.cube(mod.bar));
$ uglifyjs -V
uglify-es 3.2.1
$ cat bundle.js | uglifyjs --toplevel -bc
var mod_foo = 1, mod_bar = 2, mod_cube = x => x * x * x;

console.log(mod_foo, mod_cube(mod_bar));
$ cat bundle.js | uglifyjs --toplevel -mc passes=3
console.log(1,8);
@Andarist

This comment has been minimized.

Copy link
Contributor

@Andarist Andarist commented Dec 3, 2017

wow, that's great new 👏 is uglify-es considered to be stable now? I recall you mentioning few months back that it isn't there quite yet, but I can remember that incorrectly, so ain't sure.

Anyway - that's all and nice in a rollup world, but considering that React is bundled mostly in apps and those use mostly webpack which does not do scope hoisting by default, I'd still say that exporting an object as default should be avoided to aid other tools than uglisy-es+rollup in their efforts to produce smaller bundle sizes. Also for me it is semantically better to avoid this - what libs actually do in such cases is providing a namespace and it is better represented when using import * as Namespace from 'namespace'

@kzc

This comment has been minimized.

Copy link

@kzc kzc commented Dec 3, 2017

is uglify-es considered to be stable now?

As stable as anything else in the JS ecosystem. Over 500K downloads per week.

that's all and nice in a rollup world, but considering that React is bundled mostly in apps and those use mostly webpack which does not do scope hoisting by default

Anyway, it's an option. Webpack defaults are not ideal anyway - you have to use ModuleConcatenationPlugin as you know.

@lukastaegert

This comment has been minimized.

Copy link

@lukastaegert lukastaegert commented Dec 4, 2017

Adding a few cents here:

  • I totally agree with @Rich-Harris that semantically, named exports are the right choice
  • I really do not like either import React from 'react' or import * as React from 'react' just to be able to use JSX syntax. In my eyes, this design is clearly violating the Interface Segregation Principle in that it forces users to import all of React just to be able to use the createElement part (though admittedly with a namespace export, a bundler like Rollup will strip out the unneeded exports again)

So if we are at a point where we might make breaking-change decisions, I would advise to change this so that JSX depends on a single (global or imported) function. I would have called it createJSXElement(), which in my opinion describes it even better than createElement() and no longer needs the React context to make sense. But in a world where every byte counts, jsx() is probably ok, too.

This would also at last decouple JSX from React in a way such that other libraries can choose to support JSX by using the same transformation and supplying a different jsx function. Of course you have a lot of responsibility here guiding countless established applications through such a transformation but from an architectural point of view, this is where I think React and JSX should be heading. Using Babel to do the heavy lifting of such a transformation sounds like a great idea to me!

@Andarist

This comment has been minimized.

Copy link
Contributor

@Andarist Andarist commented Dec 4, 2017

Personally I do not see much gain in migrating to jsx helper as the default IMHO for the babel plugin should be importing it from the react package, so the name of the actual helper doesn't really matter - the rest is just matter of having it configurable.

@NMinhNguyen

This comment has been minimized.

Copy link
Contributor

@NMinhNguyen NMinhNguyen commented Dec 12, 2017

This is probably slightly tangential to the main discussion, but I'm curious how well ES modules work with checking process.env.NODE_ENV to conditionally export dev/prod bundles? For example,

if (process.env.NODE_ENV === 'production') {
module.exports = require('./cjs/react.production.min.js');
} else {
module.exports = require('./cjs/react.development.js');
}

I may be missing something obvious here, but I'm struggling to see how to translate this pattern into ES modules?

@milesj

This comment has been minimized.

Copy link
Contributor

@milesj milesj commented Dec 12, 2017

@NMinhNguyen Conditional exports aren't possible with ES modules.

@Andarist

This comment has been minimized.

Copy link
Contributor

@Andarist Andarist commented Dec 12, 2017

process.env.NODE_ENV checks can be at more granular (code) level though, ready to be replaced by the bundler with appropriate values.

@NMinhNguyen

This comment has been minimized.

Copy link
Contributor

@NMinhNguyen NMinhNguyen commented Dec 12, 2017

@Andarist @milesj Thanks for confirming my suspicion :)

process.env.NODE_ENV checks can be at more granular (code) level though, ready to be replaced by the bundler with appropriate values.

From the React 16 blog post I thought that the process.env.NODE_ENV checks were pulled out to the very top on purpose (as opposed to them being more granular, which is what they are in the source, if I'm not mistaken), to help performance in Node.js?

Better server-side rendering

React 16 includes a completely rewritten server renderer. It's really fast. It supports streaming, so you can start sending bytes to the client faster. And thanks to a new packaging strategy that compiles away process.env checks (Believe it or not, reading process.env in Node is really slow!), you no longer need to bundle React to get good server-rendering performance.

Like, I'm not sure how one could use the module field in package.json and differentiate between dev/prod for ESM while keeping ES bundles flat and not affecting Node.js perf

@Andarist

This comment has been minimized.

Copy link
Contributor

@Andarist Andarist commented Dec 12, 2017

Like, I'm not sure how one could use the module field in package.json and differentiate between dev/prod for ESM while keeping ES bundles flat and not affecting Node.js perf

This for sure is a drawback, because there is no standard way at the moment for doing this. OTOH it's just a matter of tooling, it is possible (and it's rather easy) to compile this in build steps of your application even today. Ofc it would be easier if package could expose dev/prod builds and the resolver would just know which one to pick, but maybe that's just a matter of pushing this idea to tooling authors.

@TrySound

This comment has been minimized.

Copy link
Contributor

@TrySound TrySound commented Aug 10, 2018

Current plan is migrating all packages with only named exports. This change won't affect libraries code and shouldn't introduce breaking changes since docs uses named exports too.

For another packages we need to handle both default and named exports which work differently with various tools.

@stken2050

This comment has been minimized.

Copy link

@stken2050 stken2050 commented Aug 11, 2018

@TrySound My apologies.
I didn't mean to you, since the head mention of this topic is

We can't quite easily do this because we haven't really decided on what top-level ES exports would look like from each package. For example, does react have a bunch of named exports, but also a default export called React? Should we encourage people to import * for better tree shaking?

and, the day mentioned is a while ago, and I just thought it's been discussed in React community, so I wanted to suggest the decision would be clear. Thanks!

Ubehebe added a commit to Ubehebe/react that referenced this issue Oct 24, 2018
allowing rollup to use the existing entry points (index.js) creates an
es module bundle with a single default export. but @types/react
describes named exports. the result is that tsc doesn't allow you to
write `import react from "react"`, but if you use named exports, it
fails at runtime (since the module has no named exports).

for now, add manually curated entry points that re-export the internals.
the eventual fix should come in
facebook#11503.
@leoyli

This comment has been minimized.

Copy link

@leoyli leoyli commented Feb 13, 2019

Want get some update on this...

I'm using webpack v4 for bundling our application, while my IDE intellisense (WebStorm) suggest me to use import * as React from 'react'; while my coworker ask me to change import React from 'react'; in a code review. Both works fine so I thought he is saying some nonsense, but to make him happy I'm changing it anyway. That's also how I find this thread.

While out of curious, I compare the differences at the final build size between it (with React 16.8.1):

In import * as React from 'react';: 6,618,723 bytes
In import React from 'react';: 6,619,077 bytes

So obviously, it did have some differences, marginal though. (note. I have did the same with propTypes)

If my understanding in this thread correctly, it would be favour of having import * as React from 'react';, right?! Because (1) yes, it did save some size; (2) ESM is a standardized way so no more CJS leftovers. If that is the case I'd like to change this today and align with my IDE.

@TrySound

This comment has been minimized.

Copy link
Contributor

@TrySound TrySound commented Feb 13, 2019

@leoyli In long term yes. But first there will be both named and default exports to not break existing code.

@lukejacksonn

This comment has been minimized.

Copy link

@lukejacksonn lukejacksonn commented Mar 15, 2019

I took matters into my own hands here, somewhat as an experiment as I am not using a bundler in my projects anymore and wanted to still use react (direct from unpkg.com like you can with other libraries such as Vue, Hyperapp etc.). This is what I came up with, nothing fancy, just a hand edited umd:

https://github.com/lukejacksonn/es-react

An ES6 module exposing the latest version of React and ReactDOM

As described in the README this is mostly a POC but for people that cannot wait for this build to land then it is a 16.8.3 build which includes hooks, suspense, lazy etc. and works as expected by doing:

import { React, ReactDOM } from 'https://unpkg.com/es-react'

Maybe some of you in this thread will find it useful.. personally I have been using this approach to create a build step free react starter project. It is also still a work in progress.

@PM5544

This comment has been minimized.

Copy link

@PM5544 PM5544 commented Mar 15, 2019

@lukejacksonn We've been using such a solution on production as well, while our approach was different in the sense that it is more a transformer script for the UMD version of React and ReactDOM in your current project. And that it outputs these file separately so for the most code out there it should be a drop in replacement. If you're interested https://github.com/wearespindle/react-ecmascript and you can also load it from unpkg https://unpkg.com/react-ecmascript/

@lukejacksonn

This comment has been minimized.

Copy link

@lukejacksonn lukejacksonn commented Mar 15, 2019

@PM5544 Oh wow.. this is a much more comprehensive solution than mine! Great job 💯

@ghengeveld

This comment has been minimized.

Copy link
Contributor

@ghengeveld ghengeveld commented Mar 15, 2019

Awesome stuff @PM5544. Would love to hear more about it someday. Maybe a guest appearance back at Xebia?
I've recently adopted pack to bundle my open source packages, which supports UNPKG.
Anyone know of a good article on loading dependencies from UNPKG directly rather than using a bundler?

@lukejacksonn

This comment has been minimized.

Copy link

@lukejacksonn lukejacksonn commented Mar 15, 2019

I'm currently writing one and will be giving it as a talk at React Norway in June too!

@sdegutis

This comment has been minimized.

Copy link

@sdegutis sdegutis commented Jun 17, 2019

@TrySound Has there been any update on this since February? What's left to get this issue moving, and can I participate in getting this issue closed by some coding work? I already signed the CA, and I have time available today to work on it.

@TrySound

This comment has been minimized.

Copy link
Contributor

@TrySound TrySound commented Jun 17, 2019

This need to be merged first #15037

@sdegutis

This comment has been minimized.

Copy link

@sdegutis sdegutis commented Jun 17, 2019

@TrySound Okay thanks I have forwarded my offer for assistance to that thread.

@sokra

This comment has been minimized.

Copy link

@sokra sokra commented Jul 23, 2019

When you go with a default React export you can go with this approach:

// react/index.js
import * as React from "./react";
export { React as default }
export * from "./react";

// react/react.js
export function createElement() {}
...

This make it statically analyse-able that the default export is a namespace object which allows tree-shaking for these constructs in webpack 5 and rollup:

import React from "react";

React.createElement(); // <- only `createElement` export is used
@lukastaegert

This comment has been minimized.

Copy link

@lukastaegert lukastaegert commented Jul 23, 2019

Rollup would love that very much

@GrosSacASac

This comment has been minimized.

Copy link

@GrosSacASac GrosSacASac commented Nov 26, 2019

I am in the rollup gitter chat for 1.5 years and this kind of issues comes up every 2 weeks or so ...

@lukastaegert

This comment has been minimized.

Copy link

@lukastaegert lukastaegert commented Nov 26, 2019

BTW, @lukejacksonn did a tremendous job on the inofficial es-react fork, can highly recommend it.

@stken2050

This comment has been minimized.

Copy link

@stken2050 stken2050 commented Nov 28, 2019

BTW, @lukejacksonn did a tremendous job on the inofficial es-react fork, can highly recommend it.

I wonder why the official FB team does nothing to this.

@lukastaegert

This comment has been minimized.

Copy link

@lukastaegert lukastaegert commented Nov 28, 2019

I was also hoping this would generate some pressure to move things forward, and I guess so was @lukejacksonn himself. As I understand, though, he actually received some support from the React team to build his fork from the original sources, so there seems to be at least some interest in this.

@lukejacksonn

This comment has been minimized.

Copy link

@lukejacksonn lukejacksonn commented Nov 29, 2019

I hoped this too indeed. Actually I received next to no support from the react team in the creation of the package (besides some kind words of encouragement from @threepointone). Recently a colleague here at Formidable helped me build the package programatically which is an improvement over doing it by hand every time a new version of react is released but does result in much less clean output in the network tab so I am not sure if it will stay like this yet. We shall see!

@foisonocean

This comment has been minimized.

Copy link

@foisonocean foisonocean commented Dec 10, 2019

It's almost 2020 now, I would like to know if there are any updates from the official FB team? Would there be any changes related to this in React v17?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.