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

Use lodash v4 #611

Merged
merged 1 commit into from Jan 31, 2016
Merged

Use lodash v4 #611

merged 1 commit into from Jan 31, 2016

Conversation

@gaearon
Copy link
Contributor

gaearon commented Aug 24, 2015

Sending a PR on behalf of @jdalton :-)
Is there anything else that needs to be done here?

@salehe

This comment has been minimized.

Copy link
Contributor

salehe commented Aug 24, 2015

Technically, we are actually waiting for Lodash 4 to be released, making second task done! 😜

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Aug 24, 2015

Thanks for getting this rolling @gaearon!

I'll add some comments inline to talk through some of the changes.

@@ -66,6 +66,9 @@
"webpack": "^1.9.6",
"webpack-dev-server": "^1.8.2"
},
"dependencies": {
"lodash": "~4.0.0"

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 24, 2015

Contributor

Using the ~ because we're referencing internal modules.
We try to avoid issues with internal modules for the ~ range.

loaders: [{
test: /\.js$/,
loaders: ['babel-loader'],
include: [

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 24, 2015

Contributor

webpack recommends using include over exclude when possible.

extensions: ['', '.js']
}
plugins: [
['lodash/internal/baseIteratee.js', './toFunction.js'],

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 24, 2015

Contributor

The biggest reduction in size is from replacing baseInteratee references with toFunction to cut out the callback shorthand helpers (e.g. _.mapValues(objects, 'propName')).

This comment has been minimized.

Copy link
@wmertens

wmertens Jan 18, 2016

Contributor

Why not add these commit notes as inline comments?

This comment has been minimized.

Copy link
@jdalton

jdalton Jan 18, 2016

Contributor

Sounds good.

}
plugins: [
['lodash/internal/baseIteratee.js', './toFunction.js'],
['lodash/object/keys.js', '../internal/baseKeys.js'],

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 24, 2015

Contributor

Replacing keys and keysIn with their base forms since yall aren't concerned with treating sparse arrays as dense or compat paths for older enviros.

];

module.exports = config;
module.exports = _.merge({}, baseConfig, {

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 24, 2015

Contributor

Using _.merge here instead of Object.create(baseConfig) because the Object.create route doesn't clone object references so changes to them change the baseConfig properties.

This comment has been minimized.

Copy link
@gaearon

gaearon Aug 24, 2015

Author Contributor

👍

'process.env.NODE_ENV': '"production"'
}),
new webpack.optimize.UglifyJsPlugin({
compressor: {

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 24, 2015

Contributor

Added more uglify options to crunch it more :)

This comment has been minimized.

Copy link
@gaearon

gaearon Aug 24, 2015

Author Contributor

Oh good. I was always curious what you referred to in some older tweet about not going all the way with Uglify. :-)

['lodash/object/keysIn.js', '../internal/baseKeysIn.js']
].map(function(pair) {
var resourceRegExp = RegExp('\\b' + _.escapeRegExp(pair[0]) + '$');
return new webpack.NormalModuleReplacementPlugin(resourceRegExp, pair[1]);

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 24, 2015

Contributor

For info on why I used NormalModuleReplacementPlugin instead of resolve.alias see https://github.com/jdalton/redux/commit/7b50f3b7af340ab18663de4ca240a912c6d0a303#commitcomment-12833016

loaders: ['babel-loader'],
include: [
path.resolve(__dirname, 'src'),
path.resolve(__dirname, 'node_modules', 'lodash')

This comment has been minimized.

Copy link
@aaronjensen

aaronjensen Aug 24, 2015

Contributor

Is this necessary because of NormalModuleReplacementPlugin below?

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 24, 2015

Contributor

Is this necessary because of NormalModuleReplacementPlugin below?

I believe so. We can try removing lodash's path from it to see if it still works.

Tested. It can safely be reverted to:

loaders: [
  { test: /\.js$/, loaders: ['babel-loader'], exclude: /node_modules/ }
]
}]
},
node: {
process: false
},

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 24, 2015

Contributor

node: { process: false } prevents webpack from including the process shim because of the simple inference check in lib/utils/combineReducers.

new webpack.DefinePlugin({
'__DEV__': 'false',
'process.env.NODE_ENV': '"production"'
}),

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 24, 2015

Contributor

Using DefinePlugin for __DEV__ as well as process.env.NODE_ENV removes the use of verifyStateShape in lib/utils/combineReducers as dead code in the production build.

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Aug 24, 2015

@gaearon

  • Update “no dependencies” notice in README with something sensible

There's no need to update the notice since lodash methods will be built-in so still no external deps required.

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Aug 24, 2015

Fair enough! I guess technically it looks like an (inner) dependency, but I'll just point people to this thread. :-)

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Aug 25, 2015

Instructions to build lodash v4.

  • Clone lodash-cli master and install master deps
git clone --depth 1 https://github.com/lodash/lodash-cli
cd lodash-cli && mkdir node_modules && cd node_modules
git clone --depth 1 https://github.com/lodash/lodash
cd .. && npm i && cd ..
  • Build lodash dep for Redux
cd redux && mkdir node_modules && cd node_modules
node ../../lodash-cli/bin/lodash modularize exports=node -o ./lodash && node ../../lodash-cli/bin/lodash -d -o ./lodash/index.js
cd lodash && npm init

# <go through wizard and ensure version is 4.0.0>

cd ../../ && npm i
@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Aug 31, 2015

Just to confirm.. we're still waiting for Lodash 4 to land right?

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Aug 31, 2015

Correct.

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Aug 31, 2015

Got it, thanks!

@c2h5oh

This comment has been minimized.

Copy link

c2h5oh commented Jan 11, 2016

Lodash v4 is out :-)

lodash/lodash@9a2748c

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Jan 27, 2016

4.0.1 is out. Are we good to continue, do you think?

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Jan 30, 2016

FYI I think we might not need anything else except isPlainObject: #1329

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Jan 31, 2016

Updated. With this in place it'll be trivial to use another lodash method when needed.

Just in case the webpack.NormalModuleReplacementPlugin was something like:

  plugins: [
    ['lodash/_baseIteratee.js', './_toFunction.js'],
    ['lodash/keys.js',           './_baseKeys.js'],
    ['lodash/keysIn.js',         './_baseKeysIn.js']
  ].map(function(pair) {
    var resourceRegExp = RegExp('\\b' + _.escapeRegExp(pair[0]) + '$');
    return new webpack.NormalModuleReplacementPlugin(resourceRegExp, pair[1]);
  }).concat(
    new webpack.optimize.OccurenceOrderPlugin()
  )
@@ -1,5 +1,9 @@
'use strict';

var _ = require('lodash');
var path = require('path');

This comment has been minimized.

Copy link
@timdorr

timdorr Jan 31, 2016

Member

These lines can go. They're not used in this context.

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Jan 31, 2016

Just so it's known (no judgement)

master
redux.min.js  5.73 kB

lodash
redux.min.js  5.85 kB

I'd rather spend 130 bytes to not have to maintain a standard function not critical to Redux. And if someone is already bundling in Lodash, then the net result is actually going to be negative for them.

I don't see any big issues with this PR. LGTM.

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Jan 31, 2016

What is the post gzip difference?
(Just curious. I agree it's not significant.)

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Jan 31, 2016

2062 vs 2153 bytes, so 91 bytes.

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Jan 31, 2016

dist git(master): gzip-size redux.min.js | pretty-bytes
2.05 kB
dist git(lodash): gzip-size redux.min.js | pretty-bytes
2.14 kB

module.exports = config;
module.exports = _.merge({}, baseConfig, {
plugins: _.union(baseConfig.plugins, [

This comment has been minimized.

Copy link
@aaronjensen

aaronjensen Jan 31, 2016

Contributor

Just curious, but why use 'union' here? The plugins are all new objects, seems like basic concat would suffice.

gaearon added a commit that referenced this pull request Jan 31, 2016
Use lodash v4
@gaearon gaearon merged commit c3fe3ee into reduxjs:master Jan 31, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Jan 31, 2016

👍

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Jan 31, 2016

Interestingly, I have trouble trying to run tests on the UMD builds. If I change

--- a/test/createStore.spec.js
+++ b/test/createStore.spec.js
@@ -1,5 +1,5 @@
 import expect from 'expect'
-import { createStore, combineReducers } from '../src/index'
+import { createStore, combineReducers } from '../dist/redux'

I get

/Users/dan/p/redux/dist/redux.js:411
                var objectProto = global.Object.prototype;
                                        ^

TypeError: Cannot read property 'Object' of undefined
    at Object.<anonymous> (/Users/dan/p/redux/dist/redux.js:405:26)
    at Object.exports.__esModule (/Users/dan/p/redux/dist/redux.js:467:31)

This is new with this PR. However I'm also able to run require('./redux') from node in dist folder so I'm confused what actually broke.

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Jan 31, 2016

In tests, Mocha transpiles everything that isn't in node_modules. Previously Redux worked even when transpiled twice so I could manually test dist builds with Mocha. However after this PR it doesn't work when transpiled twice. In particular, 'use strict'; inserted by Babel breaks function () {return this;} check that Lodash does to find the global.

In general we don't want to encourage people to transpile Redux UMD build, but I wouldn't be surprised if some people do that. Feels like breaking them isn't a very nice thing to do.

@phated

This comment has been minimized.

Copy link

phated commented Jan 31, 2016

@gaearon do you think this belongs over in the lodash issue tracker? Something like "lodash breaks when transpiled twice"? If so, would you mind opening it over there for tracking? I know JD is keeping an eye on this thread but I figured it would be helpful. Thanks.

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Jan 31, 2016

@cht8687

This comment has been minimized.

Copy link
Contributor

cht8687 commented Jan 31, 2016

If we only need a few methods, and if those methods are working fine within Redux, why do we need depend on another library which may cause troubles?

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Jan 31, 2016

Because if we can avoid maintaining them, we want to.

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Jan 31, 2016

I just force-pushed to master to not include this PR. Let's figure out a solution that doesn't break double transpiled builds. If we can't, I'm happy to bump major and ship, but that would be a breaking change.

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Jan 31, 2016

@cht8687

if those methods are working fine within Redux, why do we need depend on another library which may cause troubles?

It turns out DIY solutions were a burden on the project and caused support issues. Problems that were solved by Lodash since it does utilities very well. Since Lodash is a widely used utility lib it has more eyes on it and resolves more issues making it a solid choice when choosing whether to DIY or not. Redux using Lodash further feeds Lodash's support line as any issues it has in the future will pipe directly to Lodash improving it for all other users.

@gaearon

I just force-pushed to master to not include this PR. Let's figure out a solution that doesn't break double transpiled builds. If we can't, I'm happy to bump major and ship, but that would be a breaking change.

Would you describe the issue a bit more? The unit tests passed in this PR.
If you liked it, then you shoulda put a unit test on it – Beyonce (probably)

––Update––
Moved to lodash/lodash#1916.

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Jan 31, 2016

Can you describe the issue a bit more. The unit tests passed in this PR. If you liked it, then you shoulda put a unit test on it – Beyonce (probably)

Yeah, so this is something I tend to test manually because I don't have a good solution for doing that automatically. (I agree I should create an issue for that.)

I just changed imports inside tests to point to ../dist/redux instead of ../src to make sure that the UMD build didn't regress. The UMD builds themselves are fine, but I found another issue I described in lodash/lodash#1916.

People are supposed to use UMD builds “as they are” but I know some people have bad transpiler configs that transpile third party modules. So we end up in a situation where the UMD build of Redux might be transpiled with Babel by somebody else, and it used to work until now. I'm not saying I want to support this (I'm fine to break it in a major and declare we don't want people to transpile our already transpiled UMD builds) but it used to work. However now, due to Babel inserting use strict into modules it transpiles, Lodash breaks because it depends on non-strict behavior. At least this is my understanding of the issue.

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Jan 31, 2016

So, to reproduce, you can:

  1. Build a UMD build in dist folder
  2. Go to http://babeljs.io/repl and transpile it, paste it back into the file

Expected:
3. UMD build still works despite being transpiled “again” (behavior in master)

Actual:
3. UMD build is broken due to crash in Lodash (behavior in this PR)

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Jan 31, 2016

Let's continue discussion in lodash/lodash#1916

@gaearon

This comment has been minimized.

Copy link
Contributor Author

gaearon commented Feb 1, 2016

@jdalton

Thank you very much for helping me understand this issue and how global detection works in Lodash!
I wonder if there is any way you can publish an official version of

  /** Used to determine if values are of the language type `Object`. */
  var objectTypes = {
    'function': true,
    'object': true
  };

  /**
   * Checks if `value` is a global object.
   *
   * @private
   * @param {*} value The value to check.
   * @returns {null|Object} Returns `value` if it's a global object, else `null`.
   */
  function checkGlobal(value) {
    return (value && value.Object === Object) ? value : null;
  }

  /** Detect free variable `exports`. */
  var freeExports = (objectTypes[typeof exports] && exports && !exports.nodeType) ? exports : null;

  /** Detect free variable `module`. */
  var freeModule = (objectTypes[typeof module] && module && !module.nodeType) ? module : null;

  /** Detect free variable `global` from Node.js. */
  var freeGlobal = checkGlobal(freeExports && freeModule && typeof global == 'object' && global);

  /** Detect free variable `self`. */
  var freeSelf = checkGlobal(objectTypes[typeof self] && self);

  /** Detect free variable `window`. */
  var freeWindow = checkGlobal(objectTypes[typeof window] && window);

  /** Detect the popular CommonJS extension `module.exports`. */
  var moduleExports = (freeModule && freeModule.exports === freeExports) ? freeExports : null;

  /** Detect `this` as the global object. */
  var thisGlobal = checkGlobal(objectTypes[typeof this] && this);

  /**
   * Used as a reference to the global object.
   *
   * The `this` value is used if it's the global object to avoid Greasemonkey's
   * restricted `window` object, otherwise the `window` object is used.
   */
  var root = freeGlobal || ((freeWindow !== (thisGlobal && thisGlobal.window)) && freeWindow) || freeSelf || thisGlobal || Function('return this')();

Maybe as a private Lodash module? Otherwise with this PR I'll have to trade maintaining isPlainObject for maintaining global detection which doesn't seem easier.

@jdalton jdalton mentioned this pull request Feb 1, 2016
@gaearon gaearon mentioned this pull request Feb 1, 2016
8 of 8 tasks complete
@tyscorp tyscorp mentioned this pull request Feb 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.