-
Notifications
You must be signed in to change notification settings - Fork 7k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Make
_.isNative
throw if core-js is detected.
- Loading branch information
Showing
2 changed files
with
90 additions
and
10 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e156459
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I wrote - if you will use something like that - I should make it completely undetectable for preventing possible problems. It is better to wait when I'll add marks for completely polyfilled features.
e156459
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @zloirock!
I decided to simply not allow
_.isNative
to work whencore-js
is used. No fake key detection and no chance of faulty results that way. The fake detection that is used is for internal use only. I may expand the detection to other libs that mask methods in the future.In v5, I'll elevate the error from
_.isNative
to the entire library.At that time, ~8 months,
lodash
will simply not run withcore-js
or other like packages.e156459
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we try to have a discussion about this? You both sound pissed and that's no way to have a thought-out discussion. I'm obviously quite out of the loop on this issue, but clearly we need to find some solution here so we can all be happy with.
e156459
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no worries, @loganfsmyth 🙂
With each major release Lodash pushes its modern builds a bit more. This is a progression of that is all.
e156459
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally, my reading of
is that at that point lodash could never be used alongside Babel however, is that correct, or am I misunderstanding that?
e156459
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naw. The
babel-polyfill
package is opt-in. By 2017 I'm sure something will be worked out to allow other shim layers, replace the shim layer, or revise its policies. At the very least we'll have a guide for how to run Babel with Lodash and have shims of some kind.This patch is more of a bug fix and I suspect the usage of
_.isNative
is small and even less with this specific scenario. The risk is further reduced by releasing it as a patch bump. If there are widespread issues, of course we'll consider reverting.This small change with
_.isNative
won't likely cause too much grief. Further down the line with v5, maybe. But 8 months is a long time and nothing is set in stone for v5 yet. I'm positive an opt-in polyfill package and its overreaching practices can be addressed by then.I added a doc note to help explain the change in
_.isNative
behavior.e156459
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated:
Just a heads up: The Lodash v4.13.0+ update, which includes this patch, has been live for over a week now. This release reaches 5,700+ packages as a dependency and 660+ as a dev-dependency. So far, no relevant issues have been reported.
To my knowledge the only issues reported related to this in any way were from Feb 2015 and May 2015. In both cases the root issue was
core-js
augmentingObject.prototype
methods and in both cases Lodash promptly took action to avoid problems. It was only after the May ’15 issue thatcore-js
opt-ed to fake out Lodash.