Closed WebReflection closed 1 week ago
Totals | |
---|---|
Change from base Build 11271610914: | 0.0% |
Covered Lines: | 90 |
Relevant Lines: | 90 |
On the other hand, I can just merge this and move on ... hopefully browsers will implement this soon enough to not worry about performance and Chrome is already shipping this so majority of users will be safe ... I can move on with the next thing, everyone wins, little perf deviation won't matter neither.
Let's publish this as patch 🥳
In this unfortunate thread https://github.com/mdn/content/pull/36294#issuecomment-2407752579 has been brought up the fact this ungap module is not 100% compliant with native JSON behavior.
Even if it's stated at the top of the ungap documentation that this project doesn't care about paranoia or irrelevant (slow, bloated, etc) extra checks are often not part of the offer:
it was extremely trivial to make it 100% specs compliant and as resilient as
core-js
counterpart by using (and trapping) onceReflect.apply
to guarantee that even in poisoned environment, if this module is imported soon enough everything will just work as expected.Changes
reviver
andreplacer
to reflect current native JSON expectationsReflect.apply
to be sure tainted envs can't intercept json valuesTODO
.call
and.apply
when appropriate ... it's not the goal of this module to be super defensive about everything because even incore-js
if somebody manages to poison Function.prototype.call orbind
nothing is safe, so I don't like this false claim that any polyfill out there is safer than others, it's always a matter of race conditions nobody can do anything about because that's how JS works: it's scripting, it's dynamic, one can play safe but it never really is (custom browsers with code injected AOT, extensions that might get a chance to taint the env before the page/module does, and so on)Reflect.apply
and if bad get rid of that false sense of security that gives this temporary module (until all browsers ship it natively) nothing useful at all for the real-worldThat's it ... I might as well just drop Reflct.apply already before merging this MR as I don't really like the premises to have it in.