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

[core] Output ES code in /es #8772

Merged
merged 1 commit into from
Oct 20, 2017
Merged

[core] Output ES code in /es #8772

merged 1 commit into from
Oct 20, 2017

Conversation

NeoLegends
Copy link
Contributor

@NeoLegends NeoLegends commented Oct 20, 2017

Hey!

There was some discussion over in #5952 with the result that material-ui should publish a non-transpiled / minimally transpiled version of the source code as well. This offers better build sizes to people that know their target browsers well and do the transpilation / polyfilling themselves.

I've changed the build process so that it outputs a true ES6 version (instead of just a module file) along the ES5 one. module / esnext:main-definitions have been updated accordingly.

Discussion: Is babeling to ES6 already "too much" compilation, does publishing a stage-x-free release make more sense?


The final version

We push #5952 further with a /es folder alternative of the modules using all the latest ES features.

package.json Outdated
@@ -30,7 +30,7 @@
"docs:export": "next export -o docs/export",
"prebuild": "rimraf build",
"build:es2015": "cross-env NODE_ENV=production babel ./src --ignore *.spec.js --out-dir ./build",
"build:es2015modules": "cross-env NODE_ENV=production BABEL_ENV=modules babel ./src/index.js --out-file ./build/index.es.js",
"build:es2015modules": "cross-env NODE_ENV=production BABEL_ENV=modules babel ./src --out-dir ./build/es",
Copy link
Member

Choose a reason for hiding this comment

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

We can't change this module, it's required to support old browser. Even react-redux is following our current pattern. The best we can do is ship another folder with a different babel-preset-env configuration.

Copy link
Member

@oliviertassinari oliviertassinari Oct 20, 2017

Choose a reason for hiding this comment

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

We could call this new script build:es.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also means the esnext:main / module definitions in the package.json cannot be changed, right? Couldn't this fall under the breaking changes of v1?

Copy link
Member

@oliviertassinari oliviertassinari Oct 20, 2017

Choose a reason for hiding this comment

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

Looking at https://www.reddit.com/r/javascript/comments/5jwg9c/confused_about_fields_module_and_jsnextmain_in/

I think that we can remove the esnext:main key as it's non-standard.

This also means the esnext:main / module definitions in the package.json cannot be changed, right?

Ideally, we would have two key versions in the package.json.
One for people that don't transpile the node_modules. The main key sounds like the best option.
One for people that want to transpile the node_modules. There is not option for this, as far as I know, in the modern building tools.

Also, given that transpiling the node_modules is an advanced mode of any building tool, I think simply having a folder for it is enough.

So now, comes the naming question of those folders. I think that we should keep the flat structure for our es2015 version.
What about using a direct naming convention? /es2015modules for the es2015modules script and /es for the new es script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agh, I didn't see this comment here before doing the edits yesterday.

Also, given that transpiling the node_modules is an advanced mode of any building tool, I think simply having a folder for it is enough.

Hmm, I assume that when somebody depends on the module field serving ES5 code with ES6 modules something is off. I would expect that the code thats listed under module to be at least full ES6 code, if not ES6+.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 20, 2017

I had to inline the global babel presets / plugins because babel would normally merge those which would put us back where we started generating plain old ES5.

@NeoLegends Babel is about to add a js version of the configuration file. Maybe we can take advantage of it. I'm not sure if it's in babel@6 or babel@7

Discussion: Is babeling to ES6 already "too much" compilation, does publishing a stage-x-free release make more sense?

I think that we should be releasing a pure ESnext version of the modules. Something that matches the latest official supported features of the language. Nothing more (stage-x), nothing less (require, etc).

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Oct 20, 2017
@NeoLegends
Copy link
Contributor Author

I think that we should be releasing a pure ESnext version of the modules. Something that matches the latest official supported features of the language. Nothing more (stage-x), nothing less (require, etc).

I like this idea. Seems like we need to output that do a different folder tho. Can webpack be made to automatically reference /src when importing from material-ui?

@NeoLegends
Copy link
Contributor Author

NeoLegends commented Oct 20, 2017

@oliviertassinari I updated the PR.

@NeoLegends Babel is about to add a js version of the configuration file. Maybe we can take advantage of it. I'm not sure if it's in babel@6 or babel@7

.babelrc.js seems to be a feature of Babel@7, which is not finished yet.

I think that we should be releasing a pure ESnext version of the modules. Something that matches the latest official supported features of the language. Nothing more (stage-x), nothing less (require, etc).

The build process now generates ESNext code. 🎉

Can webpack be made to automatically reference /src when importing from material-ui?

Turns out, it can, and it's very simple, too: https://webpack.js.org/plugins/normal-module-replacement-plugin/

@@ -41,7 +41,6 @@ function createPackageFile() {
name: 'material-ui',
main: './index.js',
module: './index.es.js',
'jsnext:main': './index.es.js',
Copy link
Member

Choose a reason for hiding this comment

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

As far as I know, it's legacy.

@oliviertassinari oliviertassinari added core and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Oct 20, 2017
@oliviertassinari oliviertassinari changed the title [build] Output ES6 code in build/es [core] Output ES code in build/es Oct 20, 2017
@oliviertassinari oliviertassinari changed the title [core] Output ES code in build/es [core] Output ES code in /es Oct 20, 2017
@oliviertassinari oliviertassinari merged commit ca0a4e4 into mui:v1-beta Oct 20, 2017
@oliviertassinari
Copy link
Member

@NeoLegends Thanks

@NeoLegends
Copy link
Contributor Author

NeoLegends commented Oct 21, 2017

@oliviertassinari A question: as the v1-beta branch isn't stable yet, wouldn't it be possible to do a breaking change here and use this as an opportunity to clean up the old way of handling the module-field in package.json? Seems like something is off when one uses a bundler that understands module but depends on the code being ES5.

@NeoLegends NeoLegends deleted the feature/unbuild-es2015 branch October 21, 2017 08:41
@NeoLegends NeoLegends restored the feature/unbuild-es2015 branch October 21, 2017 08:41
@NeoLegends NeoLegends deleted the feature/unbuild-es2015 branch October 21, 2017 08:42
@NeoLegends NeoLegends restored the feature/unbuild-es2015 branch October 21, 2017 08:42
@oliviertassinari
Copy link
Member

@NeoLegends We introduce breaking changes from time to time. I don't think that it's the old way of doing it. As far as I know, it's still the standard way of handling the module key.

the-noob pushed a commit to the-noob/material-ui that referenced this pull request Nov 17, 2017
@zannager zannager added the core Infrastructure work going on behind the scenes label Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants