Uh oh!
There was an error while loading. Please reload this page.
Switch to lodash + lodash-webpack-plugin ... - #5616
Conversation
| @@ -1,4 +1,4 @@ | |||
| import $ from 'jquery'; | |||
| import _ from 'lodash'; | |||
There was a problem hiding this comment.
This wasn't really necessary, it got pulled with some other changes I was exploring (using jQuery's slim build which doesn't have $.extend) ... I can revert.
| ); | ||
| }) | ||
| .value(); | ||
| let tuples = _.map(kvData, (val, key) => [val, key]); |
There was a problem hiding this comment.
The lodash webpack bundle doesn't support chaining, and lodash's .map doesn't play as nice w/ empty objects so I made this more defensive.
| }, | ||
| plugins: [ | ||
| new LodashModuleReplacementPlugin({ | ||
| shorthands: true, |
There was a problem hiding this comment.
If you can avoid the shorthands feature it will save you a considerable amount. It's one of the heaviest features.
jdalton
commented
Jun 24, 2017
You can upgrade to webpack v3 and use |
Yeah, I'll probably do that separately, and maybe after a few patch releases. And thanks for your feedback on this PR. |
I'm curious what the minified bundle numbers are now that you dropped the
Cool. When you do keep in mind the |
benvinegar
commented
Jun 24, 2017
Good to know.
I did re-run the numbers, and it turned out that by adding |
That's unminified right? Lodash has inline documentation in its unminified form so that is a falsely inflated view. You can minify as well as gzip to get a more prod-centric size. |
Also good to know. Yeah, I'm excited for the results – just too impatient to wait for |
| }); | ||
| _.each(this.state.rawProjectData, statName => { | ||
| $.each(this.state.rawProjectData, statName => { |
There was a problem hiding this comment.
couldn't se just use .forEach at this point?
There was a problem hiding this comment.
couldn't se just use .forEach at this point?
If it's an array yes. If you're only working with arrays then you also don't need the "collections" feature set.
MaxBittker
left a comment
There was a problem hiding this comment.
we need audit the other uses of underscore that didn't show up in this diff for problems like pick/pickBy (which I only saw because of the mapValues change). glad we're doing this though
| }) | ||
| .value(); | ||
| return _.map(this.props.kvData, (val, key) => [val, key]).map(([val, key]) => { | ||
| return ( |
There was a problem hiding this comment.
return _.map(this.props.kvData, (val, key) => [val, key]).map(([val, key]) => {
is equivalent toreturn _.map(this.props.kvData, (val, key) => {
| @@ -1,5 +1,6 @@ | |||
| import React from 'react'; | |||
| import underscore from 'underscore'; | |||
| import lodash from 'lodash'; | |||
There was a problem hiding this comment.
use _ here for consistency?
There was a problem hiding this comment.
If _ can be used safely in the file without conflicts, yes.
| /*eslint getsentry/jsx-needs-il8n:0*/ | ||
| import React from 'react'; | ||
| import underscore from 'underscore'; | ||
| import underscore from 'lodash'; |
There was a problem hiding this comment.
See comment above. If that can't be done, rename to lodash.
| @@ -1,5 +1,5 @@ | |||
| import moment from 'moment'; | |||
| import underscore from 'underscore'; | |||
| import underscore from 'lodash'; | |||
| import Reflux from 'reflux'; | ||
| import * as Router from 'react-router'; | ||
| import underscore from 'underscore'; | ||
| import underscore from 'lodash'; |
There was a problem hiding this comment.
this one will break the bundle's external API
| @@ -1,5 +1,5 @@ | |||
| import React from 'react'; | |||
| import underscore from 'underscore'; | |||
| import underscore from 'lodash'; | |||
| import DocumentTitle from 'react-document-title'; | ||
| import React from 'react'; | ||
| import underscore from 'underscore'; | ||
| import lodash from 'lodash'; |
| @@ -1,5 +1,5 @@ | |||
| import iOSDeviceList from 'ios-device-list'; | |||
| import {isString} from 'underscore'; | |||
| import {isString} from 'lodash'; | |||
There was a problem hiding this comment.
do we get static analysis benefits from importing like this?
There was a problem hiding this comment.
In this case, not really since we're using this webpack plugin.
| // We only want to send back the values which weren't disabled | ||
| let data = _.mapObject( | ||
| let data = _.mapValues( | ||
| _.pick(options, option => !option.field.disabled), |
There was a problem hiding this comment.
pick doesn't doesn't work like this in lodash, we need to do _.pickBy for it to accept a predicate.
There was a problem hiding this comment.
If so, please change (and good catch). Let's also test this page (I definitely did not, sigh).
| import React from 'react'; | ||
| import DocumentTitle from 'react-document-title'; | ||
| import underscore from 'underscore'; | ||
| import underscore from 'lodash'; |
There was a problem hiding this comment.
just commenting '_' on these because i feel obligated now
There was a problem hiding this comment.
I would also say it makes it less confusing for new people to know where to google documentation
MaxBittker
commented
Jul 12, 2017
https://github.com/lodash/lodash/wiki/Migrating was super useful 👍 |
MaxBittker
left a comment
There was a problem hiding this comment.
my concerns are addressed in my commits
MaxBittker
commented
Jul 13, 2017
Do not merge yet. at least comments are broken |
MaxBittker
commented
Jul 25, 2017
update: react-mentions relies on some features we're disabling in LodashModuleReplacementPlugin |
MaxBittker
commented
Jul 25, 2017
figuring out what those are in order to proceed |
MaxBittker
commented
Jul 25, 2017
this is deploying to staging right now for good measure, but I think it's safe to merge. @benvinegar@billyvg |
billyvg
commented
Jul 25, 2017
We should probably add an acceptance test for the activity tab, since our suite did not catch these errors |
MaxBittker
commented
Jul 25, 2017
you're right billy, also not the first time this page has hidden a regression now that I think. wrote one off the cuff so I'll see what percy shows |
| new LodashModuleReplacementPlugin({ | ||
| collections: true, | ||
| currying: true, | ||
| flattening: true |
There was a problem hiding this comment.
We talked about having a comment about mentioning currying and flattening are enabled to support react-mentions. Can we add that?
* Removes underscore in favor of lodash * Uses lodash-webpack-plugin + babel-lodash to minimize filesize impact * Saves ~100kb unminified between removing underscore (~50kb) and using lodash-webpack-plugin over full lodash dep (another ~50kb) Note: need to make corresponding changes in getsentry repo, since it uses a global alias to sentry's exported `underscore` global.
This reverts commit a9d890f4d8d3a338d22b49f2ead9b8c888277700.
This reverts commit cda5a13f099f3703b91f1d2b750e13b782e6ffdd.
This reverts commit a5c1080eb2f285d3e22cc6e7a954241d2464ceb7.
MaxBittker
commented
Jul 26, 2017
I'm not sure how you were measuring the bundle sizes, webpack -p gave me: |
MaxBittker
commented
Jul 26, 2017
|
Undoes the work introduced in #5616. This plugin adds around 40-50 seconds to webpack build times and has caused confusion (see #13834). Also adds an eslint rule to prevent importing `lodash` bundle: getsentry/eslint-config-sentry#46 | -- | With Plugin | Without Plugin | Difference | lodash-es | lodash-es + react-mentions | | -- | ----------- | -------------- | ---------- | --------- | - | | app.js | 739051 | 739120 | 0% | | | | app.js.gzip | 184017 | 184040 | 0% | | 186949 | | vendor.js | 2076311 | 2090876 | +0.7% | 2089352 | | vendor.js.gizp | 375718 | 380446 | + 1.2% | 380630 | 380476 | | build time (w/ stats) | 316s | 466s | + 47% | | | build time (env=prod) | 335s | 198s | n/a | | Currently, there is no sizable difference between `lodash-es` and `lodash` -- we should revisit this in the future.
A big reason we did this was because we have some dependencies which were requiring lodash anyways, which meant we were bundling both lodash and underscore together 🤐.
I experimented with a bunch of dependency changes:
Interestingly, removing underscore doesn't result in as much benefit as you'd expect (vs optimizing the lodash build alone), because those functions just end up being transferred to the lodash build.
Note: need to make corresponding changes in getsentry repo, since it uses a global alias to sentry's exported
underscoreglobal.cc @dcramer
#nochanges