Skip to content
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

Conversation

yuri-karadzhov
Copy link
Contributor

@yuri-karadzhov yuri-karadzhov commented Jul 22, 2019

Q                       A
Fixed Issues? Fixes #10247
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link babel/website#2071
Any Dependency Changes? No
License MIT

Add static parameter to t.classsProperty builder

t.classProperty(key, value, typeAnnotation, decorators, computed, static);

@JLHwung JLHwung added pkg: types PR: New Feature 🚀 A type of pull request used for our changelog categories labels Jul 22, 2019
@JLHwung
Copy link
Contributor

JLHwung commented Jul 22, 2019

@yuri-karadzhov Could you squash into a single commit?

@yuri-karadzhov
Copy link
Contributor Author

@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.

@JLHwung
Copy link
Contributor

JLHwung commented Jul 22, 2019

@yuri-karadzhov You can add babel repo as a new remote upstream and rebase on the upstream/master.

git remote add upstream https://github.com/babel/babel
git fetch upstream
git rebase upstream/master
git push origin --force

@yuri-karadzhov
Copy link
Contributor Author

@JLHwung after git rebase upstream/master it says Current branch add-static-to-class-property-builder is up to date.

@JLHwung
Copy link
Contributor

JLHwung commented Jul 22, 2019

@yuri-karadzhov Assume you don't have new commits, you can rebase onto upstream/master, it will discard the two merge commit:

git rebase HEAD~1 --onto upstream/master
git push origin --force

@yuri-karadzhov yuri-karadzhov force-pushed the add-static-to-class-property-builder branch from a348f81 to dc9ff74 Compare July 22, 2019 07:05
@yuri-karadzhov
Copy link
Contributor Author

@JLHwung Thanks a lot for your help! I did not work with forks for quite a while.

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 22, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11227/

@yuri-karadzhov yuri-karadzhov force-pushed the add-static-to-class-property-builder branch from dc9ff74 to 68d8ca4 Compare July 22, 2019 15:34
@JLHwung
Copy link
Contributor

JLHwung commented Jul 23, 2019

@babel/core-team Could you restart the build? The test failure comes from temporary 500 response from yarnpkg.com.

"type": "StringLiteral",
"value": "test",
},
"static": null,
Copy link
Contributor

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

computed: {
default: false,
validate: assertValueType("boolean"),
},

Copy link
Contributor Author

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:

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.

Copy link
Contributor

@JLHwung JLHwung Jul 24, 2019

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@yuri-karadzhov yuri-karadzhov force-pushed the add-static-to-class-property-builder branch from 68d8ca4 to 4963b19 Compare July 24, 2019 08:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: types PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

t. classProperty should allow to create static class property
5 participants