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

Add es2015 modules export, build umd with rollup and remove generated code #359

Merged
merged 4 commits into from
Aug 17, 2017

Conversation

tusbar
Copy link
Contributor

@tusbar tusbar commented Aug 16, 2017

The commonjs build is kept in the repository today (lib). Is there any reason for that? If so, would you want the es build to be in it too?


This allows to get rid of the bundled core-js from babel-runtime. And allows for rollup/webpack scope hoisting.
I’m seeing a >6KB gzipped gain on etalab/inspire with this.

@PaulLeCam
Copy link
Owner

Thanks for taking this on, looks great!
I'll try it out this week if I find some time, at worst this week-end.

Regarding lib being in the repository, it's only for convenience of being able to pull specific branches from GitHub and consume them directly, I agree it's not needed so much and it doesn't seem to be common practice so I don't mind getting rid of it.

Is there any reason for not including the babel-runtime plugin in the es build? Is it because it is assumed it will be processed by the app's build system anyways?
Also, at the moment the src folder is exported to npm so that the source can be consumed directly, would it still be useful or does the es export completely cover this possible use case?

Thanks!

@tusbar
Copy link
Contributor Author

tusbar commented Aug 16, 2017

@PaulLeCam thank you for your fast input on this.

Regarding lib and dist in the repository, if you believe that it is in the scope of this PR, I would be happy to remove it. :)

Concerning babel-runtime, I have been trying to read as much as possible on the matter, and I don’t see it anywhere being used in react component libraries (react-router-dom, redux, react-redux, react-intl, react-i18next, etc.)

As far as I understand, babel-transform-babel-runtime gets rid of all the helpers generated on a per-module basis and uses the ones defined in babel-runtime (which gets aliased as core-js). It also adds scoped polyfills for Set, Map, Symbol, Promise, etc., and none seem to be used in react-leaflet. It will also include the regenerator runtime when using generators or async functions, which is not the case in react-leaflet.

So I am not sure it’s even needed for the commonjs build. The output seems lighter with babel-runtime but it requires about 6.5KB gzipped of babel-runtime.

As for the es/jsnext build, the idea is to expose the same transpiled code, without transforming the import/exports.

@tusbar
Copy link
Contributor Author

tusbar commented Aug 16, 2017

So I’ve removed babel-transform-runtime as I do not believe that it is necessary (I’ll revert if you tell me otherwise).

I’ve made sure that all babel configuration lies in .babelrc (umd configuration was duplicated in webpack.babel.config) – which required the webpack configuration to stop using es6 modules.

And I’ve enabled scope hoisting for the umd build (that removes unnecessary closures) to lighten the umd builds.

@PaulLeCam
Copy link
Owner

Whao that's great, thanks!

I originally added babel-runtime to avoid the repeated helpers definition in all the modules, but if in the end the added runtime size is larger better no have it indeed!

Regarding the folders let's add dist and lib to the .gitignore and src to the .npmignore assuming the added es folder covers the use case, it should make things cleaner.

With the changes to the .babelrc file, do the webpack.config.babel.js in the examples folder still work or should it be changed to CommonJS as well please?

Great job on improving the UMD builds as well! Do you think using Rollup instead of Webpack would make sense as well, or provide more benefits?
Webpack could still be used to run the examples and have Rollup for the UMD builds if that makes sense?

@PaulLeCam
Copy link
Owner

One other thing I'm thinking could be good to improve tree shaking would be to use lodash-es instead of lodash, possibly using a plugin to replace the imports automatically as react-redux is doing?

@tusbar
Copy link
Contributor Author

tusbar commented Aug 16, 2017

@PaulLeCam I have removed the generated code from the repository and switched the umd build to use rollup. (Yeah I thought about doing that, but thought it would be too much for this PR – I already had the rollup config lying around 😄). The rollup build is much lighter than the current build of webpack.

Before, with webpack:

$ ll dist
total 688
-rw-r--r--  1 tusbar  staff   263K Aug 16 21:30 react-leaflet.js
-rw-r--r--  1 tusbar  staff    77K Aug 16 21:30 react-leaflet.min.js

After, with rollup (and lodash-es):

$ ll dist
total 504
-rw-r--r--  1 tusbar  staff   192K Aug 16 21:31 react-leaflet.js
-rw-r--r--  1 tusbar  staff    50K Aug 16 21:31 react-leaflet.min.js

Regarding lodash and lodash-es, I’m all for it.

Remove src out of the npm package.
@tusbar tusbar force-pushed the es-build branch 2 times, most recently from 6393020 to 300cfcc Compare August 16, 2017 22:06
@tusbar
Copy link
Contributor Author

tusbar commented Aug 16, 2017

Ok, I’ve cleaned up the commits and moved everything in order.
Let me know if everything is here.

To ease the review, the first commit only removes all the generated code (and updates .{git,npm}ignore), everything else happens in the last 3 commits. :)

@tusbar tusbar changed the title Add es2015 modules export Add es2015 modules export, build umd with rollup and remove generated code Aug 16, 2017
tusbar added 3 commits August 17, 2017 00:34
Remove babel-plugin-transform-runtime from the build as it is not
necessary and increases the output size for nothing.
Lightens the umd builds quite a bit:

    $ ll dist old
    dist:
    total 504
    -rw-r--r--  1 tusbar  staff   192K Aug 16 21:16 react-leaflet.js
    -rw-r--r--  1 tusbar  staff    50K Aug 16 21:16 react-leaflet.min.js

    old:
    total 600
    -rw-r--r--  1 tusbar  staff   263K Aug 16 21:11 react-leaflet.js
    -rw-r--r--  1 tusbar  staff    77K Aug 16 21:11 react-leaflet.min.js
This was taken from react-redux. :)
@PaulLeCam
Copy link
Owner

It looks great!
I'll try it out a bit this week-end before merging to master but I can't see of any issue with the changes, thanks for this PR!

@PaulLeCam PaulLeCam merged commit c4ef757 into PaulLeCam:master Aug 17, 2017
@tusbar tusbar deleted the es-build branch August 17, 2017 20:47
@tusbar
Copy link
Contributor Author

tusbar commented Aug 17, 2017

Thank you for merging this is @PaulLeCam!

Will you create a new release for it? :)

@PaulLeCam
Copy link
Owner

Thanks for your PR, it's great changes!

I'll probably make a new release in the next few days, I'm working on updating the types to match the changes in Flow 0.53 so when it's done it should be good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants