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

Remove general lodash require? #249

Closed
5 tasks done
aaronjensen opened this issue Nov 22, 2016 · 17 comments
Closed
5 tasks done

Remove general lodash require? #249

aaronjensen opened this issue Nov 22, 2016 · 17 comments

Comments

@aaronjensen
Copy link
Member

aaronjensen commented Nov 22, 2016

As mentioned in #136, lodash is required and included globally for use within examples. I personally don't use this and don't care to pay the 100kb+ price in size and load time. Would it be possible to make this optional or just not include it and let people require what they want in examples?

Is there currently a hook to add globals like this? If so, I'd probably like to use it for a few other things.

  • Find working solution to provide context modules.
  • Replace global React/Lodash with it.
  • Add an option to easily add globals.
  • Remove Lodash at all.
  • Update docs.
@sapegin
Copy link
Member

sapegin commented Nov 22, 2016

Probably it‘s a good idea. But probably current way to optionally enable it is a bit too complicated. You need to add a JS file to entries like this:

globals._ = require('lodash');

@sapegin
Copy link
Member

sapegin commented Nov 22, 2016

But probably we can use webpack.DefinePlugin or webpack.ProvidePlugin for that and make an easy config option like that:

globals: {
  _: require('lodash'),
}

I’d use that too! Not sure that globals is the best name.

@sapegin
Copy link
Member

sapegin commented Nov 22, 2016

But don’t compare ungzipped sizes, they are not that important ;-)

@aaronjensen
Copy link
Member Author

I've never used webpack.DefinePlugin for anything but constants, but maybe that'd work. I think the entries is more straight forward. It's what I'm doing today for some other stuff.

ungzipped sizes are important for runtime afaik. The more code that's there, gzipped or not, the more it needs to execute which will slow startup.

@sapegin
Copy link
Member

sapegin commented Nov 22, 2016

@aaronjensen
Copy link
Member Author

I'm not sure that will work for the case we want. It doesn't actually set them globally, it makes them available as free variables w/in modules, which our examples are not.

@sapegin
Copy link
Member

sapegin commented Nov 23, 2016

Looks like it works fine. I’ve added a todo list to the issue. Do you want to work on that?

@sapegin sapegin added this to the 5.0.0 milestone Nov 23, 2016
@oliverturner
Copy link
Collaborator

An alternative would be to use babel-plugin-lodash to automate the module extraction: possibly less onerous.

@sapegin
Copy link
Member

sapegin commented Nov 23, 2016

We can’t use Babel for user’s code and especially for examples. Styleguidist itself already use crerry-picking everywhere.

@oliverturner
Copy link
Collaborator

got it: good catch :)

@aaronjensen
Copy link
Member Author

@sapegin 4ae9927 doesn't work at all for me:

react style guide example 2016-11-23 07-30-23

@sapegin
Copy link
Member

sapegin commented Nov 23, 2016

Very strange. Why does it work for me then? ;-}

@aaronjensen
Copy link
Member Author

I'm guessing because of the new build step. If you didn't compile after making the change it'd still look like it works.

I think that what we actually want to do is add React to the execution context. I've not yet wrapped my head around the way that it all works, but that'd be the best imo. That way we wouldn't pollute the global namespace.

Furthermore, the config option to add "globals" should maybe be called context and add those things to the context as well rather than globally:

context: {
  _: 'lodash',
  foo: 'foo',
}

@sapegin
Copy link
Member

sapegin commented Nov 23, 2016

You're genius! I forgot to start the compiler 🐙

And I agree on both points: about context and about the name.

sapegin added a commit that referenced this issue Nov 24, 2016
#249

BREAKING CHANGE:

Lodash will not be available in examples as `_`.
sapegin added a commit that referenced this issue Nov 24, 2016
@sapegin sapegin mentioned this issue Nov 24, 2016
9 tasks
@sapegin
Copy link
Member

sapegin commented Nov 24, 2016

Before:

❯ gz examples/basic/styleguide/build/bundle.js
Original: 2.35 MB
Gzipped:  499 kB (21.29%)

After:

❯ gz examples/basic/styleguide/build/bundle.js
Original: 1.81 MB
Gzipped:  403 kB (22.31%)

Almost 100 KB difference gzipped!

@sapegin sapegin closed this as completed Nov 24, 2016
@sapegin
Copy link
Member

sapegin commented Nov 24, 2016

And with fixed UglifyJS (and with proper name mangling options):

❯ gz examples/basic/styleguide/build/bundle.js
Original: 631 kB
Gzipped:  183 kB (29.02%)

@aaronjensen
Copy link
Member Author

Very nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants