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

Build umd with rollup #2283

Merged
merged 4 commits into from
Mar 8, 2017
Merged

Build umd with rollup #2283

merged 4 commits into from
Mar 8, 2017

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Mar 7, 2017

Ref #2245

@TrySound
Copy link
Contributor Author

TrySound commented Mar 7, 2017

40 461 b -> 27 793 b
10 738 b -> 7 503 b

@gaearon
Copy link
Contributor

gaearon commented Mar 7, 2017

CI seems to fail because of ES3ify check.
But while we're at it, why does the compiled bundle have .default anywhere at all?

@TrySound
Copy link
Contributor Author

TrySound commented Mar 8, 2017

Nice catch. symbol-observable has only jsnext:main which is disabled by default.

@timdorr
Copy link
Member

timdorr commented Mar 8, 2017

@gaearon

But while we're at it, why does the compiled bundle have .default anywhere at all?

That is the way Babel 6 handles export default. @kentcdodds wrote about it, but here's why:

// foo.js
const foo = {baz: 42, bar: false}
export default foo
// bar.js
import {baz} from './foo' // <-- Not allowed!

That is against the spec (though it may have changed since 6.0.0), so they "broke" that behavior to fix the spec. Hence, all defaults are exported in the default property of the module.

@gaearon
Copy link
Contributor

gaearon commented Mar 8, 2017

Right, but if we're bundling with rollup, we need to let it handle ES modules instead of Babel. So we should change the rollup env to not do the commonjs transform.

@gaearon
Copy link
Contributor

gaearon commented Mar 8, 2017

@TrySound Can you post resulting bundles as gists for comparison?

@timdorr
Copy link
Member

timdorr commented Mar 8, 2017

BTW, we're down to:

26843 dist/redux.js
 7159 dist/redux.min.js

👍

Here's the latest build artifacts for easy perusal: https://gist.github.com/timdorr/73a8d368c1d0c3d3c83aa73af8f02cbe

As compared to before: https://unpkg.com/redux@3.6.0/dist/redux.js

@timdorr
Copy link
Member

timdorr commented Mar 8, 2017

LGTM. Most of the cost savings are from the babel import/export transforms and the inter-module imports (__webpack_require__ stuff). Surprisingly hefty! Here's hoping webpack 2 can add scope hoisting soon...

@gaearon
Copy link
Contributor

gaearon commented Mar 8, 2017

This part worries me a little. Does it inline in prod build?

@TrySound
Copy link
Contributor Author

TrySound commented Mar 8, 2017

Yeah. This increases bundle. Weird behavior.

@gaearon
Copy link
Contributor

gaearon commented Mar 8, 2017

Could be #2244, I'm not sure if it was fixed yet in master.

@timdorr
Copy link
Member

timdorr commented Mar 8, 2017

Now:

26843 dist/redux.js
 7023 dist/redux.min.js

Keep going!

@TrySound
Copy link
Contributor Author

TrySound commented Mar 8, 2017

@gaearon Yeah. Current min release is smaller because of that thing. Inference with code elimination is too hard :) That PR should be reverted.

@timdorr
Copy link
Member

timdorr commented Mar 8, 2017

I'll merge it in here and then merge in this PR. The reason for #2030 are something you can fix with tooling.

@TrySound
Copy link
Contributor Author

TrySound commented Mar 8, 2017

Do you guys want to release it as 3.* or 4.*?

@timdorr
Copy link
Member

timdorr commented Mar 8, 2017

@TrySound Can you enable contributor pushing to your branch? Should be a checkbox at the top of the PR.

@timdorr
Copy link
Member

timdorr commented Mar 8, 2017

Once I can push it, reverting that takes us down to:

26636 dist/redux.js
 5827 dist/redux.min.js

Now we're cooking.

@TrySound
Copy link
Contributor Author

TrySound commented Mar 8, 2017

Yay!

@timdorr It's enabled.

@timdorr
Copy link
Member

timdorr commented Mar 8, 2017

Cool, I think we're good. This doesn't change anything outside of the UMD build and does so in entirely a compatible way, so I think it only needs a patch version. Correct me if I'm wrong, otherwise I'll push this as 3.6.1. I don't believe we've had any other non-docs/examples updates since that release.

@gaearon
Copy link
Contributor

gaearon commented Mar 8, 2017

It's probably safe as a patch. Maybe as a minor for a tiny bit of extra safety.

@TrySound
Copy link
Contributor Author

TrySound commented Mar 8, 2017

Okay. I just would like to remove jsnext:main which is legacy. Rollup uses module field now. But guys still add this field looking at this and similar packages. But it will be breaking change.

@timdorr
Copy link
Member

timdorr commented Mar 8, 2017

Go ahead and do that against the next branch, which is our target for 4.0 stuff.

AFAIK, webpack is following suit, so that should be a low-impact breaking change for most folks.

@timdorr timdorr merged commit bf13473 into reduxjs:master Mar 8, 2017
@timdorr
Copy link
Member

timdorr commented Mar 8, 2017

BTW, thanks for the hustle on this!

@TrySound TrySound deleted the rollup-umd branch March 8, 2017 01:37
seantcoyote pushed a commit to seantcoyote/redux that referenced this pull request Jan 14, 2018
* Build umd with rollup

* Resolve jsnext entry in symbol-observable

* Remove useless commonjs

* Don't try to handle a missing process.env
This pull request was closed.
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.

3 participants