Reflect #313
Reflect #313
Conversation
|
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: Is there any JS engine that's implemented any part of |
|
|
||
| // Reflect | ||
| if (!globals.Reflect) { | ||
| globals.Reflect = { |
There was a problem hiding this comment.
Reflect itself should be set with defineProperty rather than an ! check, to ensure that enumerability is correct, and to allow for later overriding if a faulty implementation is detected.
In addition, all the properties on Reflect should be added with defineProperties, for the same reason.
|
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 :-) |
|
@zloirock thanks, good call! |
|
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. |
|
Awesome, when I do my testing, I'll test with IE 11 as well. |
… Firefox behaviour
|
I think I'm primarily waiting for |
…afeApply fo internal use. Will make a PR for es-abstract.
| return internal_get(target, key, receiver); | ||
| }), | ||
|
|
||
| getOwnPropertyDescriptor: throwUnlessTargetIsObject(Object.getOwnPropertyDescriptor), |
There was a problem hiding this comment.
The es6-shim needs to work in ES3 engines, which don't have property descriptors, a shimmable setPrototypeOf, Object.isExtensible, and a number of others.
Each Reflect method that can't work as-is in shimmed ES3 needs to be conditionally added.
There was a problem hiding this comment.
The definition of Reflect is inside the if (supportsDescriptors) block along with Set and Map. Therefore I suppose that the descriptor support exists. Can't I be sure Object.getOwnPropertyDescriptor exists?
There was a problem hiding this comment.
Ah, I didn't realize. In that case, then the comment changes to be, any Reflect methods that don't require descriptors should be defined even when they're not supported :-) but that can be a followup, since that at least won't be broken anywhere. Thanks for clarifying!
|
OK, we're down to the primary blocker being that any Thanks a lot for being persistent on this, this is going to be great to have! |
|
Latest spec draft adds |
|
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 |
|
I set out with the intention of polyfilling Reflect in an ES5 environment, since it is so dependent on the Object property methods. |
|
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 |
|
This should be it. 👍 |
|
@Victorystick It looks great and I'm ready to merge it, but I still see a merge commit (37e8818) - can you do a rebase onto latest master (as opposed to a merge with latest master)? |
|
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). |
(From local rebased 'reflect' branch)
|
Merged via dd6b1b2 |
|
Thanks so much for contributing this, and working through all the back and forths with me! |
|
No worries. Thank you! |
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.