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

Revisit dependencies and add browser ready build #112

Closed
wants to merge 2 commits into from

Conversation

dariocravero
Copy link
Contributor

This PR:

  • cleans up package.json by removing the unused browserify section;
  • removes envify and babel-runtime as they aren't being used;
  • adds a dist build script (borrowed from flummox) to create distribution ready builds of redux that can be used in a browser directly, put into a cdn, into a live playground :), etc. @acdlite is there any specific reason why flummox doesn't save (nor ignore on .gitignore) dist/?

Regarding sizes, we're looking at 56k for a regular build and ~12k for a minified one, when gzipped it's a bit below 4k! Pretty impressive! :)

Darío Javier Cravero added 2 commits June 16, 2015 23:23
script (reusing flummox's one) so that we can output a dist-ready
version of it.

Fixes reduxjs#115. Fixes reduxjs#88.
@dariocravero dariocravero changed the title Remove unnecessary browserify config in package.json and envify dependency Revisit dependencies and add browser ready build Jun 16, 2015
@gaearon
Copy link
Contributor

gaearon commented Jun 16, 2015

I'm going to look at this in a few days, really busy at the moment. Thanks a lot!

@dariocravero
Copy link
Contributor Author

Happy to help :) I've updated the PR's original message to better reflect what it's doing

@chicoxyzzy
Copy link
Contributor

don't sure we need babel-runtime. see #114 (comment)

@dariocravero
Copy link
Contributor Author

@chicoxyzzy this PR removes it. It depends on Symbols I guess then :)

@chicoxyzzy
Copy link
Contributor

👍 than :)

@gaearon
Copy link
Contributor

gaearon commented Jun 17, 2015

I see a lot of lodash code there. Weird. Are we using deep equality somewhere?
It would be cool if you could generate webpack module tree and look what's dragging heavy deps.

Thanks!

@ooflorent
Copy link
Contributor

I'm -1 on pushing dist/ to the repository because:

  • it adds noise
  • it will be a pain to merge PRs that include dist files

However adding dist to the NPM package and adding metadata for cdnjs would be a better alternative.

@gaearon
Copy link
Contributor

gaearon commented Jun 17, 2015

I'm with @ooflorent here, let's put dist into .gitignore (but keep it in the NPM).

@dariocravero
Copy link
Contributor Author

Good :) Will do that later on today

On Wed, Jun 17, 2015 at 12:18 PM Dan Abramov [email protected]
wrote:

I'm with @ooflorent https://github.com/ooflorent here, let's put dist
into .gitignore (but keep it in the NPM).


Reply to this email directly or view it on GitHub
#112 (comment).

@emmenko
Copy link
Contributor

emmenko commented Jun 17, 2015

I'm -1 on pushing dist/ to the repository

👍

@gaearon
Copy link
Contributor

gaearon commented Jun 17, 2015

Good :) Will do that later on today

Thanks! Can you also please generate the module tree and check which Lodash modules drag the most dependencies?

@frankychung
Copy link
Contributor

I wrote some quick utils to replace mapValues, isPlainObject, and pick and using Webpack's module tree here's what I found:

  • Default: 53 lodash modules
  • Without mapValues: 27
  • Without pick: 48
  • Without isPlainObject: 51

@gaearon
Copy link
Contributor

gaearon commented Jun 17, 2015

@frankychung Great!

Let's replace Lodash's way-too-smart mapValues with a simple

function mapValues(obj, fn) {
  return Object.keys(obj).reduce((result, key) => {
    result[key] = fn(result[key]);
  }, {});
}

(written in browser, might be wrong :-)

@frankychung
Copy link
Contributor

I made some changes to yours to make all the tests pass:

function mapValues(obj, fn) {
  return Object.keys(obj).reduce((result, key) => {
    result[key] = fn(obj[key], key);
    return result;
  }, {});
}

@gaearon
Copy link
Contributor

gaearon commented Jun 17, 2015

Oh right.

@frankychung Can I ask you to submit a separate PR just for this change? The function should be put into utils folder. (And a corresponding test needs to be created.)

@dariocravero
Copy link
Contributor Author

@frankychung's change does bring the size down considerably :).
Here are some extra steps we can take to make it even smaller:

pick. With lodash:

redux [master*] $ ./scripts/browser
Hash: 0b37951b3c3274654a94
Version: webpack 1.9.11
Time: 733ms
   Asset     Size  Chunks             Chunk Names
redux.js  33.7 kB       0  [emitted]  main
    + 34 hidden modules
Hash: 5be2c2076259f86ade32
Version: webpack 1.9.11
Time: 1073ms
       Asset     Size  Chunks             Chunk Names
redux.min.js  8.39 kB       0  [emitted]  main
    + 34 hidden modules

Without it:

redux [master*] $ ./scripts/browser
Hash: f36b8ded209ff7db645c
Version: webpack 1.9.11
Time: 683ms
   Asset     Size  Chunks             Chunk Names
redux.js  11.4 kB       0  [emitted]  main
    + 10 hidden modules
Hash: 1d93b4cfc6e17916ab32
Version: webpack 1.9.11
Time: 883ms
       Asset     Size  Chunks             Chunk Names
redux.min.js  4.58 kB       0  [emitted]  main
    + 10 hidden modules

That's a clear win :). Here's a rough pick implementation I have in my head, will finish it in a bit and submit a PR:

// src/utils/pick.js
export default function pick(obj, fn) {
  return Object.keys(obj).filter(key => fn(key)).reduce((result, key) => {
    result[key] = obj[key];
    return result;
  }, {});
}

Then, for redux/react (with React as an external) we should probably implement identity and isPlainObject.

As is:

# redux-react.js
Hash: 5584c7ae9a0bef8bc512
Version: webpack 1.9.11
Time: 880ms
         Asset     Size  Chunks             Chunk Names
redux-react.js  41.8 kB       0  [emitted]  main
    + 32 hidden modules
# redux-react.min.js
Hash: 5584c7ae9a0bef8bc512
Version: webpack 1.9.11
Time: 1290ms
         Asset     Size  Chunks             Chunk Names
redux-react.js  12.6 kB       0  [emitted]  main
    + 32 hidden modules

Without isPlainObject:

# redux-react.js
Hash: 1fb6dfe032a39826ccf6
Version: webpack 1.9.11
Time: 861ms
         Asset     Size  Chunks             Chunk Names
redux-react.js  23.5 kB       0  [emitted]  main
    + 12 hidden modules
# redux-react.min.js
Hash: 1fb6dfe032a39826ccf6
Version: webpack 1.9.11
Time: 1149ms
         Asset     Size  Chunks             Chunk Names
redux-react.js  9.52 kB       0  [emitted]  main
    + 12 hidden modules

isPlainObject is a bit more convoluted to shim but should be doable too. Depending on our support matrix -I imagine it's the latest browsers?- we may be able to push it even further.

identity almost has no impact (not that I was expecting any :)) but if we remove everything else from lodash we may as well remove it too to only depend on invariant.

Thoughts?

@dariocravero
Copy link
Contributor Author

@frankychung while looking back into the comments I realised I missed this:

I wrote some quick utils to replace mapValues, isPlainObject, and pick

Are you submitting isPlainObjet and pick too?

@frankychung
Copy link
Contributor

@dariocravero Sure, I'll open another PR for my implementations (and quickly replace identity)

@frankychung frankychung mentioned this pull request Jun 18, 2015
@goatslacker
Copy link

My 2 cents on dist: there's some benefit in putting it on github, if not this repo then some other repo, just so you can take advantage of cdn.rawgit

Keeping it out of this repo is good though, it just adds noise to every PR.

@dariocravero
Copy link
Contributor Author

@goatslacker @ooflorent's proposal was to push it to cdnjs when published to npm. I'll wait for @frankychung's PR (thanks:)) it land and I'll fix this one to remove any unused dependencies for good :).

@gaearon
Copy link
Contributor

gaearon commented Jun 18, 2015

Superseded by #142.

@gaearon gaearon closed this Jun 18, 2015
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.

7 participants