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
Add static to class property builder #10248
Add static to class property builder #10248
Conversation
@yuri-karadzhov Could you squash into a single commit? |
@JLHwung Not sure how to do it now (rebase does not help). Two additional commits were created when syncing my fork with babel master, feels like I did it in a wrong way. |
@yuri-karadzhov You can add babel repo as a new remote
|
@JLHwung after |
@yuri-karadzhov Assume you don't have new commits, you can rebase onto upstream/master, it will discard the two merge commit:
|
a348f81
to
dc9ff74
Compare
@JLHwung Thanks a lot for your help! I did not work with forks for quite a while. |
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11227/ |
dc9ff74
to
68d8ca4
Compare
@babel/core-team Could you restart the build? The test failure comes from temporary 500 response from |
"type": "StringLiteral", | ||
"value": "test", | ||
}, | ||
"static": null, |
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.
Could static
default to false
? Like we did on computed
babel/packages/babel-types/src/definitions/es2015.js
Lines 409 to 412 in fced5ce
computed: { | |
default: false, | |
validate: assertValueType("boolean"), | |
}, |
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 is defined as optional in classMethodOrPropertyCommon
just above computed
:
babel/packages/babel-types/src/definitions/es2015.js
Lines 405 to 408 in fced5ce
static: { | |
validate: assertValueType("boolean"), | |
optional: true, | |
}, |
Does it have sense to change definitions there or rather fields in ClassProperty
definition? Tests are passed in both cases, I think changing classMethodOrPropertyCommon
has more sense.
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 default static
was false
but later changed to optional
in #5856. At that moment static
class property were still in staged-2. I believe optional: true
is used to convey that static
field is used by TypeScript only at that time.
Now as we have static field/method in standard, I think it make sense to set a default value for static
.
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.
Yah, all that's needed is static: { default: false }
. It should add assertValueType("boolean")
automatically.
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.
I'll keep assertValueType("boolean")
for consistency with the rest of the code.
68d8ca4
to
4963b19
Compare
Add static parameter to
t.classsProperty
builder