Skip to content

Add static to class property builder#10248

Merged
nicolo-ribaudo merged 1 commit intobabel:masterfrom
yuri-karadzhov:add-static-to-class-property-builder
Jul 25, 2019
Merged

Add static to class property builder#10248
nicolo-ribaudo merged 1 commit intobabel:masterfrom
yuri-karadzhov:add-static-to-class-property-builder

Conversation

@yuri-karadzhov
Copy link
Copy Markdown
Contributor

@yuri-karadzhov yuri-karadzhov commented Jul 22, 2019 •
edited by nicolo-ribaudo
Loading

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
Copy Markdown
Contributor

JLHwung commented Jul 22, 2019

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

@yuri-karadzhov
Copy link
Copy Markdown
Contributor Author

yuri-karadzhov commented Jul 22, 2019

@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
Copy Markdown
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
Copy Markdown
Contributor Author

yuri-karadzhov commented Jul 22, 2019

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

@JLHwung
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

yuri-karadzhov commented Jul 22, 2019

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

@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Jul 22, 2019 •
edited
Loading

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
Copy Markdown
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.

JLHwung
JLHwung reviewed Jul 23, 2019
View reviewed changes
packages/babel-types/test/builders/experimental/__snapshots__/classProperty.js.snap Outdated
"type": "StringLiteral",
"value": "test",
},
"static": null,
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung Jul 23, 2019

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"),
},

Copy link
Copy Markdown
Contributor Author

@yuri-karadzhov yuri-karadzhov Jul 23, 2019

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.

Copy link
Copy Markdown
Contributor

@JLHwung JLHwung Jul 24, 2019 •
edited
Loading

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
Copy Markdown
Member

@jridgewell jridgewell Jul 24, 2019

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
Copy Markdown
Contributor Author

@yuri-karadzhov yuri-karadzhov Jul 24, 2019

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
@nicolo-ribaudo nicolo-ribaudo merged commit 4506590 into babel:master Jul 25, 2019
This was referenced Sep 17, 2019
Closed
Closed
Open
Open
Open
This was referenced Oct 3, 2019
Open
Open
Open
Merged
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 24, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jridgewell jridgewell jridgewell left review comments

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

t. classProperty should allow to create static class property

5 participants

@yuri-karadzhov @JLHwung @babel-bot @jridgewell @nicolo-ribaudo