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. #1339

Merged
merged 1 commit into from Feb 1, 2016
Merged

Use lodash v4. #1339

merged 1 commit into from Feb 1, 2016

Conversation

jdalton
Copy link
Contributor

@jdalton jdalton commented Feb 1, 2016

Attempt 2 of #611.

{
apply: function apply(compiler) {
compiler.parser.plugin('expression global', function expressionGlobalPlugin() {
this.state.module.addVariable('global', "(function() { return this; }()) || Function('return this')()")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious how much going to great lengths like Lodash does is buying us. Should we settle for this, or should we do a more sophisticated check like #611 (comment)?

Also, are we breaking anyone with this? Are there environments where Object.getPrototypeOf (as we had before) would “just work” but Function('return this')()" would fail? I'm thinking stuff like Firefox extensions etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You stated that double compiling is an edge case. One that webpack clearly doesn't support. This is going above and beyond to support double compiling in the primary browser case. If they are double compiling and using their code in extensions then yes it could be an issue.

Lodash's UMD is erm... robust because folks do some crazy interesting stuff and we have to support it. I don't know any other project who has as robust a UMD as lodash. I think your UMD is fine.

gaearon added a commit that referenced this pull request Feb 1, 2016
@gaearon gaearon merged commit 6acf979 into reduxjs:master Feb 1, 2016
@gaearon
Copy link
Contributor

gaearon commented Feb 1, 2016

Thanks for following through with this!

@jdalton
Copy link
Contributor Author

jdalton commented Feb 1, 2016

Np. Future versions of the lodash modules may remove the global reference too.

The next Lodash release will remove the global references. See lodash/npm/isPlainObject.js.

@phated
Copy link

phated commented Feb 1, 2016

@jdalton when will that version be released, can it be a 4.1.1 that is released soon?

@jdalton
Copy link
Contributor Author

jdalton commented Feb 1, 2016

Moved discussion to #1346.

jbovenschen added a commit to jbovenschen/redux that referenced this pull request Feb 10, 2016
After merging reduxjs#1339 this project does have a dependency.

I think it is better to remove the "and has no dependencies" from the README.md.
Maybe it would be better to explain why lodash is added as a dependency.
@jbovenschen jbovenschen mentioned this pull request Feb 10, 2016
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.

None yet

3 participants