paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 389 forks source link

Reflect #313

Closed Victorystick closed 9 years ago

Victorystick commented 9 years ago

The Reflect global is, as far as I know, a part of the ES6 spec. I was a bit surprised not to find it in the es6-shim. Is there any reason as to why it isn't? If not, I'd consider giving it a go.

I'd like to use this PR to discuss Reflect implementation.

ljharb commented 9 years ago

The vast majority of the things on Reflect simply can't be shimmed, as I recall - it needs to support Proxies, Symbols, etc - all sorts of unshimmable things.

I'm happy to add it if: a) everything we provide on it is 100% spec-compliant b) No engine implements Proxy, Symbol, etc before implementing any applicable Reflect methods c) Somebody testing for the presence of global Reflect won't run into trouble with only some of the methods defined. d) It works cross-realm in the appropriate fashions (which iirc is just iframes atm)

Is there any JS engine that's implemented any part of Reflect yet?

ljharb commented 9 years ago

I stopped halfway through the tests because it seems like you wanted to discuss the PR before adding the rest of them - this is awesome, and I really appreciate the work you put in!

Let's make sure we've fleshed out consequences here in this thread first, but let's proceed with the goal of merging this ASAP in mind :-)

ljharb commented 9 years ago

@zloirock thanks, good call!

Victorystick commented 9 years ago

Looking at ES6 Compat table, it seems that Reflect has been implemented at least partially by the IE team in their Technical Preview and by EchoJS.

ljharb commented 9 years ago

Awesome, when I do my testing, I'll test with IE 11 as well.

Victorystick commented 9 years ago

Reflect is getting more or less done. Time for another look?

ljharb commented 9 years ago

Sweet, will take another look today!

ljharb commented 9 years ago

@Victorystick OK, I think all the things I commented on are minor, the biggest being that both the es6-shim and its tests need to run all the way down to ES3 browsers, so everything that's not shimmable in ES3 or ES5 needs to be feature-detected at runtime. Almost there :-)

Thanks, this looks awesome! I'm excited to merge this (after the changes above, and after I do some cross-browser testing)

Victorystick commented 9 years ago

Have you also noticed that the Travis CI builds often fail because sh: 1: jscs: not found?

ljharb commented 9 years ago

Yes :-/ that's an issue with jscs's dependency tree. For now I just manually restart those builds and they pass eventually.

ljharb commented 9 years ago

I think I'm primarily waiting for bindContext to be removed before a final rereview and merge.

ljharb commented 9 years ago

OK, we're down to the primary blocker being that any Reflect method that can't work in ES3 (ie, ones that use property descriptors, or setPrototypeOf, etc) need to be conditionally added.

Thanks a lot for being persistent on this, this is going to be great to have!

zloirock commented 9 years ago

Latest spec draft adds newTarget argument to Reflect.construct. It can't be fully polyfilled, but result should be created from newTarget.prototype.

ljharb commented 9 years ago

Anything that can't be fully polyfilled in any engine needs to go in the es6-sham, not the es6-shim. However, if it can be fully polyfilled in true ES5 engines (via Object.create) then that's great

Victorystick commented 9 years ago

I set out with the intention of polyfilling Reflect in an ES5 environment, since it is so dependent on the Object property methods.

ljharb commented 9 years ago

OK - so this looks good to go, as long as it's freshly rebased on top of the latest master! ( ゚◡゚)/

Followup item is to pull as much Reflect functionality as possible into ES3 engines.

Victorystick commented 9 years ago

This should be it. :+1:

ljharb commented 9 years ago

@Victorystick It looks great and I'm ready to merge it, but I still see a merge commit (37e8818db3b4e03a9b341106381db9a1cc38b012) - can you do a rebase onto latest master (as opposed to a merge with latest master)?

ljharb commented 9 years ago

I'm anxious to merge this, so unless you're already in progress I'll make a new branch and rebase it myself (your name will remain on all the commits).

ljharb commented 9 years ago

Merged via dd6b1b254fb6e08045a9f0bdce3620457581528f

ljharb commented 9 years ago

Thanks so much for contributing this, and working through all the back and forths with me!

Victorystick commented 9 years ago

No worries. Thank you!