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
Mark class prototype as read-only #12115
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit a054296:
|
Since we already use an IIFE for classes, can we put the |
Yes, I'm working on it. Just not classes are not always wrapped with IIFE. Sometimes, when there is Current status it is Work in Progress, would it be better to convert it to draft or it is ok as it is? |
I have converted this to a draft, please mark it as ready when you need a review! |
What do you think about moving |
This PR is very outdated. And, seems I don't know what to do. If we will fix this behavior, then seems something that relates on it may be broken. Mostly I think of Object.getOwnPropertyDescriptor(fn, 'prototype').writable === false; |
Isn't this exactly what this PR is trying to fix? |
Sorry. I'm interested. Just was unable to find some time to finish. |
Not urgent! |
This seems like a good solution, IMHO. Seems easiest to implement and probably best for bundle size too. This bug is a problem for anyone trying to polyfill ECMAScript built-in objects, because polyfills transpiled by Babel will have a writable prototype. Because the problem only shows up in transpilation, it's hidden from Test262 so is hard to discover for polyfill authors. We just bumped into this issue in the Temporal polyfill. tc39/proposal-temporal#1965 (comment) Would be great to see a fix for this issue. |
a054296
to
5e412a6
Compare
5e412a6
to
86714ca
Compare
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/50265/ |
86714ca
to
934516d
Compare
934516d
to
3442057
Compare
83e8aef
to
9d71363
Compare
@@ -14,6 +14,9 @@ | |||
function _createClass(Constructor, protoProps, staticProps) { | |||
if (protoProps) _defineProperties(Constructor.prototype, protoProps); | |||
if (staticProps) _defineProperties(Constructor, staticProps); | |||
Object.defineProperty(Constructor, "prototype", { | |||
writable: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The writable
defaults to false
when a property is defined by Object.defineProperty
. Since helper is shipped in user code. We can just call
Object.defineProperty(Constructor, "prototype", {})
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this function can be shimmed, and writability might not be supported (in es5-sham, it throws), this is a good change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It defaults to false
only if the property does not exist yet:
function F() {}
Object.defineProperty(F, "prototype", {});
Object.getOwnPropertyDescriptor(F, "prototype").writable; // true
This change is an attempt to follow the spec for
.prototype
property ofclass
: necessity to followwritable: false
.The following repo describes that bug: https://github.com/denis-churbanov/babel-bug
There is also description of necessary fixes.
And here is Issue #2025 with exactly the same description.
And might be it worth looking to the following link: reactjs/rfcs#178
More detailed description here: facebook/react#4599 (comment)
But it is not about react team in general, it is much more related to Follow the Spec.
The spec purports the following code working:
And after transpilation everything becomes
function
, so...And all we need to do to fix it is to add bit more code:
So this change is about to bring some vision of solution.