Add defensive args parsing to stop FUD around this module#2
Merged
Conversation
Pull Request Test Coverage Report for Build 11308578147Details
💛 - Coveralls |
Member
Author
|
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 🥳 |
This was referenced Oct 14, 2024
Merged
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this unfortunate thread mdn/content#36294 (comment) 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-jscounterpart by using (and trapping) onceReflect.applyto guarantee that even in poisoned environment, if this module is imported soon enough everything will just work as expected.Changes
reviverandreplacerto reflect current native JSON expectationsReflect.applyto be sure tainted envs can't intercept json valuesTODO
.calland.applywhen appropriate ... it's not the goal of this module to be super defensive about everything because even incore-jsif somebody manages to poison Function.prototype.call orbindnothing 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.applyand 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.