Skip to content

Conversation

jdalton
Copy link
Contributor

@jdalton jdalton commented Feb 1, 2016

Attempt 2 of #611.

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.

3 participants