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] Use out-file-extension to build esm version #19285

Closed
wants to merge 4 commits into from

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Jan 18, 2020

Updated @babel/cli to make use of https://babeljs.io/docs/en/babel-cli#set-file-extensions when building the ESM version of the packages.
Thanks @eps1lon

This enables treeshaking on all levels and simplifies the build script since we don't need to create a package.json file in the subfolders anymore.
Even though we consider 3rd level imports private, there is nothing stopping people from using them, so they do. This will benefit them with reduced bundle sizes

For backwards compatability it still builds the esm folder which we can remove in v5

@merceyz merceyz requested a review from eps1lon January 18, 2020 13:07
@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Jan 19, 2020
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Nice, that was the original plan with this cli feature. I'm hesitant shipping even more files though. As far as I can tell e.g. /core/Button/Button.mjs and /core/esm/Button/Button.js are now identical?

Considering that we have to realize that the internal file structure is used for aliases (be it webpack or typescript) I would rather wait for v5 and simplify the structure while working on #18447.

The goal is to have { "type": "commonjs" } and .d.ts, .js, .mjs (and .node) in a single folder. But since this concerns build setups (which nobody likes to touch) I would caution against touching it until the next major.

Unless this solves an existing issue?

@TrySound
Copy link
Contributor

This will create the issue with webpack. material-ui-pickers will be forced to use commonjs version of material-ui. Moreover webpack is very confused when finds mjs extension in package.json "module" field. I recommend to not deal with mjs unless you setup package.json "exports" field

@eps1lon
Copy link
Member

eps1lon commented Jan 20, 2020

Moreover webpack is very confused when finds mjs extension in package.json "module" field. I recommend to not deal with mjs unless you setup package.json "exports" field

Not my experience with webpack. I even tested it months ago. Could you link to an existing issue or reproduction?

@TrySound
Copy link
Contributor

One of them. The main problem is overusing "default" exports.
webpack/webpack#7973

@eps1lon
Copy link
Member

eps1lon commented Jan 21, 2020

One of them. The main problem is overusing "default" exports.
webpack/webpack#7973

That repro does not contain any .mjs files. The example with a module entry seems to mimic rollup output which is considered correct? This is how we would do it.

Please provide a complete example illustrating the issue you described. You were pretty fast with that conclusion so a repro should be fast to set up as well.

@merceyz
Copy link
Member Author

merceyz commented Jan 23, 2020

As far as I can tell e.g. /core/Button/Button.mjs and /core/esm/Button/Button.js are now identical?

Yes, that is correct

I would rather wait for v5 and simplify the structure while working on #18447.

That's totally fine by me.

Unless this solves an existing issue?

Enables treeshaking on the 3rd level, so it would solve the issues that are opened due to people importing from that level and getting duplicated modules.

@oliviertassinari oliviertassinari added this to the v5 milestone Jan 23, 2020
@eps1lon
Copy link
Member

eps1lon commented Jan 26, 2020

Thanks for the work 👍

Closing for housekeeping though. Will revisit for v5 as we don't want to publish more files than necessary. We want to keep the count as low as possible for codesandbox (ci and actual).

@eps1lon eps1lon closed this Jan 26, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 26, 2020

👍 for taking http://packagephobia.now.sh/ size into consideration.

@merceyz merceyz deleted the esm branch March 7, 2020 14:26
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.

4 participants