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

Bundle cjs and es formats #2358

Merged
merged 4 commits into from
Nov 3, 2017
Merged

Bundle cjs and es formats #2358

merged 4 commits into from
Nov 3, 2017

Conversation

TrySound
Copy link
Contributor

Sorry, I have confused branches. I think this will be a breaking change and should be merge to next branch.

@gaearon
Copy link
Contributor

gaearon commented Apr 13, 2017

Can you show build output comparison?

@TrySound
Copy link
Contributor Author

@timdorr
Copy link
Member

timdorr commented Apr 13, 2017

I'm not sure what this gets us though. We only need to compile out ES.next stuff for these builds, as they're only used in the context of a module-aware system (node, webpack, rollup, etc). We don't need to bundle for these systems.

@TrySound
Copy link
Contributor Author

It's about optimization for bundlers and node.
/cc @Rich-Harris

@timdorr
Copy link
Member

timdorr commented Apr 13, 2017

Sure, but that will show up in stack traces as just redux.js, which doesn't map back to the source code in the repo, which would make it harder to trace errors and develop fixes.

Also, if the bundle is bad in some way, we wouldn't know, as the tests run against the code as-is (transpiled through babel), not in its bundled form.

Optimization should come at the end of the bundle process, not at the library level. Our early optimization might conflict with a later optimization or prevent one from occurring. We should just ship code and let the user bundle and optimize however they want.

I get where you're coming from, but unfortunately, this is going to cause more problems than it might solve.

@timdorr timdorr closed this Apr 13, 2017
@gaearon
Copy link
Contributor

gaearon commented Apr 13, 2017

I dunno, isn't this exactly what we're doing for React?

@TrySound
Copy link
Contributor Author

That's why I opened this PR. I think debug is not so significant here. Redux is small library. Function names are more than descriptive. Producing smaller packages, smaller in count of files, we are forcing the most optimal versions of libraries independent of build tools. Better UX
You care about bundle quality? I guess uglify is the most blackboxed item of stack today.
That's my opinion.

@gaearon
Copy link
Contributor

gaearon commented Apr 13, 2017

I agree with @TrySound and I would like this to be reconsidered. There is very little surface area in Redux, and IMO the argument around tests just means we need better integration tests for result bundles. Personally I’m coming to the conclusion that the end user can’t be trusted to optimize efficiently, and also doesn’t have enough leverage (e.g. can’t easily remove junk code at the module boundaries if they use Webpack).

@gaearon gaearon reopened this Apr 13, 2017
@gaearon
Copy link
Contributor

gaearon commented Apr 13, 2017

@TrySound Can you suggest a testing approach that would work well? IMO it would be nice to have a way to run all existing tests on result bundles in addition to the source. They should be testing the public API anyway.

@timdorr
Copy link
Member

timdorr commented Apr 14, 2017

I dunno, isn't this exactly what we're doing for React?

No?
screen shot 2017-04-13 at 10 30 57 pm

I'm still a pretty firm 👎 on this change. I don't know if any other library bundling for anything other than UMD.

@TrySound
Copy link
Contributor Author

@timdorr It's. Try [email protected]

@gaearon
Copy link
Contributor

gaearon commented Apr 14, 2017

No?

Sorry, I meant React 16. I thought we were pretty vocal about moving to Rollup and flat bundles :P

@Rich-Harris
Copy link

Rich-Harris commented Apr 14, 2017

Forgive me for weighing in — have been passively aware of this conversation since I was @-ed yesterday.

My view is that libraries should always bundle their own source code unless they have a compelling reason not to — @gaearon rightly points out that generating the best possible output (fewest bytes, quickest startup) requires a certain level of expertise, and for the app developer to be using particular tools. It's quite easy to end up with various unnecessary overhead such as unnecessarily transpiled import declarations, or the cruft that most bundlers introduce at module boundaries.

By pointing main and module at a bundle you're guaranteeing that Redux users get the best possible outcome, regardless of their level of expertise.

@timdorr you raise two excellent points:

  • Debugging — for Redux developers, this can be solved by generating a sourcemap and installing source-map-support before tests run. For Redux consumers diagnosing errors in their app, it's a little harder because they would have to deal with sourcemaps themselves (and you would have to include them in the repo). It's probably not a huge deal in Redux's case since it's only a few hundred LOC anyway
  • Testing — Personally I've come round to the view that tests should cover the distributable code rather than individual modules anyway, because that excludes the possibility of bugs in your build process causing issues that show up in people's apps and not in your test suite (I've been there...). It does mean you're limited to testing the public API rather than unit tests of internal modules, but then again the API is the only thing that really matters!

Just my 2c — I don't have a dog in this fight but thought I'd try and articulate why I think this is a good direction for the ecosystem to go in.

@timdorr
Copy link
Member

timdorr commented Apr 14, 2017

Sorry, I meant React 16

Ah, OK. I haven't yet tried out the alphas myself. But I would point out React has different design and performance goals than Redux. To be honest, the "well, it's what React does" argument doesn't ever sway me. React can afford to jump through certain hoops because the benefits are often multiplied as they make their way downstream (both in code and the ecosystem surrounding it).

And yes, Redux is orders of magnitude smaller and simpler, so there is less potential "damage" from things like this. However, I don't see the benefit in making the build, testing, and debugging processes more complex for no practical benefit to any stakeholder. So, since this is central to my concerns, I'll ask it more directly: what specific and practical gains does library bundling get us?

Anecdotally, I would never apply this to a library like React Router, as one of the bundle-slimming techniques we use is encouraging direct imports (import Router from 'react-router/Router') as sort of a poor man's tree shaking. Bundling everything together, including unused components, would have a negative effect on bundle sizes in that case.

@gaearon
Copy link
Contributor

gaearon commented Apr 15, 2017

since this is central to my concerns, I'll ask it more directly: what specific and practical gains does library bundling get us?

Clean, readable code without junk and indirections.

Output like this is suboptimal:

if (!(0, _isPlainObject2['default'])(action)) {
  throw new Error('Actions must be plain objects. ' + 'Use custom middleware for async actions.');
}

Especially in hot paths (we have to dereference ['default'] every time).

This also doesn't look great:

	exports.__esModule = true;
	exports.compose = exports.applyMiddleware = exports.bindActionCreators = exports.combineReducers = exports.createStore = undefined;

	var _createStore = __webpack_require__(2);

	var _createStore2 = _interopRequireDefault(_createStore);

	var _combineReducers = __webpack_require__(7);

	var _combineReducers2 = _interopRequireDefault(_combineReducers);

	var _bindActionCreators = __webpack_require__(6);

	var _bindActionCreators2 = _interopRequireDefault(_bindActionCreators);

	var _applyMiddleware = __webpack_require__(5);

	var _applyMiddleware2 = _interopRequireDefault(_applyMiddleware);

	var _compose = __webpack_require__(1);

	var _compose2 = _interopRequireDefault(_compose);

	var _warning = __webpack_require__(3);

	var _warning2 = _interopRequireDefault(_warning);

	function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { 'default': obj }; }

when it all could’ve been top-level functions referencing each other.

I’m not sure I understand the concerns about testing and debugging. How often do we release Redux updates? How fast is it changing? I can understand the concern if there is a lot of churn, but I don’t quite see why this is an issue here. Testing the output bundles will only make the process more solid (and help avoid release bugs).

@TrySound
Copy link
Contributor Author

I guess internal and interface testing are both possible with babel. We can import interface part from entry point and internal stuff from source files. That's what I did here.

@TrySound
Copy link
Contributor Author

One other point for bundling is babel. It adds helpers in every file. Rollup adds 'em only once. So for end user is not possible to make optimal build other then babel source files of the library. And this is awful UX. Because of this I reduced component based library recompose in two times. It's not small redux.

@timdorr
Copy link
Member

timdorr commented Apr 15, 2017

Clean, readable code without junk and indirections.

Ok, now I'm starting to get it 😄

I’m not sure I understand the concerns about testing and debugging.

I'm only concerned with stack traces pointing back to code that's different than what they see in the repo. Namely, if they see createStore.js:123 vs redux.js:456, they might have a hard time learning what's going on. Again, it's less of a concern for Redux where the codebase is so small.

I guess a lot of my concerns stem from couching this discussion on code running in the browser. But I'm running of steam wanting to talk through transpiled code compatibility issues...

Anyways, I guess this looks good to me. Even though we don't guarantee it, this will break anyone using direct imports. Would we want to break this behavior on a minor or major? If major, then can I rebase this PR against next. @TrySound? I don't think there are too many potential conflicts, so it should be save to do so.

@TrySound
Copy link
Contributor Author

Yeah, I think it should be released as major.

@gaearon
Copy link
Contributor

gaearon commented Apr 15, 2017

Even though we don't guarantee it, this will break anyone using direct imports

That’s the only part that’s concerning me. I’d like to have a good way forward there. What should libraries depending on specifically createStore use? Should we tell them “use Webpack 2” and make sure tree shaking works (in ES build)? Also how do we make an ES build?

@timdorr
Copy link
Member

timdorr commented Apr 25, 2017

OK, I'm going to edit this to be against next. There will likely be conflicts because I haven't yet rebased against the latest master changes (particularly, rollup for UMD). I'll try to get to that shortly, so don't worry about resolving those if they come up.

@timdorr timdorr changed the base branch from master to next April 25, 2017 16:01
@timdorr
Copy link
Member

timdorr commented Apr 25, 2017

Oh yeah, ignore all that interim junk 😄

@timdorr
Copy link
Member

timdorr commented Apr 25, 2017

OK, there we go. Merge conflicts resolved.

What should libraries depending on specifically createStore use?

We could keep transpiling individual files into lib as an interim solution. The bundled versions would be preferred, based on package.json. But those wanting direct imports could still do as before. And when they can upgrade to a tree-shaking bundler, they can do away with direct imports then.

Also how do we make an ES build?

I'm confused, isn't that already in this? It's just bundled together, but the import/exports remain untouched. The reason for the ES builds is to support tree-shaking bundlers, so they wouldn't be doing direct imports and would be using whatever the module entry point specifies.

@timdorr
Copy link
Member

timdorr commented Nov 3, 2017

Fixed up some merge conflicts and got this using the latest rollup config syntax.

For sizing comparison on the current next branch:

 ⇶ wc lib/* lib/utils/*
     63     308    2323 lib/applyMiddleware.js
     50     286    2038 lib/bindActionCreators.js
    144     840    6317 lib/combineReducers.js
     35     134     927 lib/compose.js
    266    1373    9873 lib/createStore.js
     45     200    1944 lib/index.js
      0       0       0 lib/utils
     13      61     381 lib/utils/actionTypes.js
     24     103     694 lib/utils/warning.js
    640    3305   24497 total

And this PR:

 ⇶ wc lib/*
  566  3100 22032 lib/redux.js

So, a nice size reduction, and therefore a bit of complexity reduction too.

Here's the latest output: https://gist.github.com/timdorr/f6d76d41508bf5a352846ba685322a6c

@gaearon gaearon mentioned this pull request Nov 3, 2017
8 tasks
@timdorr timdorr added this to the 4.0 milestone Nov 3, 2017
@timdorr
Copy link
Member

timdorr commented Nov 3, 2017

OK, I'm feeling good on this. It's well-tested and has a sensible reasoning for the change. We'll be sure to communicate that change to users when it lands in 4.0. Thanks, @TrySound!

@timdorr timdorr merged commit 651f5fa into reduxjs:next Nov 3, 2017
timdorr pushed a commit that referenced this pull request Nov 16, 2017
* Bundle cjs and es formats

* Test generated bundle

* Missed a yarn command in the merge process

* Updates to the rollup config.
seantcoyote pushed a commit to seantcoyote/redux that referenced this pull request Jan 14, 2018
* Bundle cjs and es formats

* Test generated bundle

* Missed a yarn command in the merge process

* Updates to the rollup config.
tomipaul pushed a commit to tomipaul/redux that referenced this pull request Apr 8, 2018
* Bundle cjs and es formats

* Test generated bundle

* Missed a yarn command in the merge process

* Updates to the rollup config.
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.

4 participants