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

isNative issues #1197

Closed
zloirock opened this issue May 11, 2015 · 7 comments
Closed

isNative issues #1197

zloirock opened this issue May 11, 2015 · 7 comments
Labels

Comments

@zloirock
Copy link

  1. lodash uses Object#toString as template for isNative assuming that is native function. But ES6 changed Object#toString logic for support Symbol.toStringTag. core-js from first versions adds polyfill for this logic and replaces Object#toString. _.isNative works wrong for all native functions with core-js / babel and some other libraries:
require('core-js');
require('lodash').isNative(Object);   // => false
require('lodash').isNative(Function); // => false
require('lodash').isNative(isNaN);    // => false

I will add fallback for lodash in next version core-js, but lodash should use native function whose logic shouldn't be changed in ES6+.
2. lodash uses isNative for detect Set, WeakMap, some object methods - but why? In most polyfills, native methods wrapped for correct work by latest spec and, with this check, will not work with lodash. Related #981. Better use correct feature detection.

@jdalton
Copy link
Member

jdalton commented May 11, 2015

Yap, I picked Object.prototype.toString because it's used heavily in other libs and not likely to be mucked with as it's on the Object.prototype. I didn't think about those mucking with it using defineProperty. Any recommendations on a method you wouldn't likely touch?

@zloirock
Copy link
Author

I didn't think about those mucking with it using defineProperty.

Wrapping Object.prototype.toString not requires native defineProperty - it's already non-enumerable.

Object.prototype.toString = 42;
for(var key in {})console.log('key: ' + key); // nothing

+ DontEnum bug.

Any recommendations on a method you wouldn't likely touch?

Should be in ES3, not constructor, not Object.prototype method - someone wrapped them for store non-enum properties or something else, not Array.prototype method - most of them replaced in es6-shim, not Math method - also for better accuracy. Should be as simple as possible. Possible, global isNaN or isFinite, their ES6 versions - static Number methods, but not sure.

@jdalton
Copy link
Member

jdalton commented May 11, 2015

There's bugs in at least some engines where overwriting a built-in does make it enumerable. So there's that.

Are you working with @ljharb on your shims?

@jdalton jdalton closed this as completed May 11, 2015
@zloirock
Copy link
Author

There's bugs in at least some engines where overwriting a built-in does make it enumerable.

Yep, but in old irrelevant engines :)

About es6-shim - it's not adds @@toStringTag logic and not wrapped Object#toString, but second issue similar:

require('es6-shim');
require('lodash').isNative(Object.keys); // => false
require('lodash').isNative(Object.getPrototypeOf); // => false
require('lodash').isNative(Object.preventExtensions); // => false

@jdalton
Copy link
Member

jdalton commented May 11, 2015

I like using an Object.prototype method as reference because Object.prototype is seen as off-limits to many devs. I'll kick around using constructor, propertyIsEnumerable, or toLocaleString.

@zloirock
Copy link
Author

They gotta make sure when they wrap they look like the built-in :)

Instead of Object#toString - it's impossible - current isNative uses Function#toString.call(method). Using .bind for fake native source will break #name property.

I'll kick around using constructor or toLocaleString.

I remember one library that wrapped Object#toLocaleString (non-enumerable method in Object.prototype for old IE). constructor will be replaced if someone wrap Object constructor. About propertyIsEnumerable - possible, I will wrap it for Symbol polyfill (all shimmed symbols are non enumerable). I think, possible use hasOwnProperty - for replace it you'd be crazy :)

@jdalton
Copy link
Member

jdalton commented May 11, 2015

Ah ya good old hasOwnProperty. I'll make that change then. I'm fine with isNative not being 100% bullet proof I just want to put a good effort in.

jdalton added a commit that referenced this issue May 11, 2015
…IsNative` to avoid issues with core-js. [closes #1197]
@jdalton jdalton added the bug label May 11, 2015
@lodash lodash locked and limited conversation to collaborators May 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

2 participants