Skip to content

fix transform-typescript logic to remove definite fields #12149

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.

Sign up for GitHub

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

Jump to bottom
Merged
existentialism merged 1 commit into from
Oct 8, 2020
Merged

fix transform-typescript logic to remove definite fields #12149

existentialism merged 1 commit into from
Oct 8, 2020

Conversation

@akphi
Copy link

@akphi akphi commented Oct 7, 2020 •
edited
Loading

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

I made sure to separate the handling for 2 cases: fields defined with the keyword declare and fields defined using the definite assignment operator.

For the latter, when the flag allowDeclareFields is true (can be treated equivalent to Typescript useDefineForClassFields = true) we will not remove the field.

Also, since I separate the handling, I adjust the error message thrown by each case slightly when the fields in these 2 cases are given values.

Also, I added a test for allowDeclareFields = false to ensure old behavior works fine

@nicolo-ribaudo What you said about babel's allowDeclareFields is similar to Typescript's useDefineForClassFields is interesting. Should we update the doc for transform-typescript here?

@babel-bot
Copy link
Collaborator

babel-bot commented Oct 7, 2020

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 7, 2020

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 4d0782f:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@akphi akphi changed the title fix transform-typescript logic to not remove definite fields when all… fix transform-typescript logic to remove definite fields Oct 7, 2020
@akphi
Copy link
Author

akphi commented Oct 8, 2020

@nicolo-ribaudo please, could you take a look when you have some time?

@nicolo-ribaudo nicolo-ribaudo added area: typescript PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Oct 8, 2020
nicolo-ribaudo
nicolo-ribaudo approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Oct 8, 2020

Should we update the doc for transform-typescript here?

Do you want to open a PR to the babel/website repo to add it to this list? https://babeljs.io/docs/en/babel-plugin-transform-typescript#typescript-compiler-options

@akphi
Copy link
Author

akphi commented Oct 8, 2020 •
edited
Loading

@existentialism @JLHwung @nicolo-ribaudo I'm new to contributing, is there anything else I need to do to get this merged?

And afterward, do you have a timeline in mind when we can release this change?

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Oct 8, 2020

We have a 2-:heavy_check_mark: policy. After another approval, we can merge this PR and release it in the first next version (7.11.8 or 7.12.0).

@existentialism existentialism merged commit f6bd749 into babel:main Oct 8, 2020
@existentialism
Copy link
Member

existentialism commented Oct 8, 2020

@akphi thanks!

@akphi
Copy link
Author

akphi commented Oct 8, 2020

@nicolo-ribaudo @existentialism Thanks! I was about the make the changes you suggest 😀 Not too familiar with the release cycle of babel myself, when do you have an estimate for when will this happen?

@existentialism
Copy link
Member

existentialism commented Oct 8, 2020

Soon :)

@akphi akphi mentioned this pull request Oct 8, 2020
Merged
@JLHwung JLHwung mentioned this pull request Oct 14, 2020
Closed
1 task
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 8, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@existentialism existentialism existentialism approved these changes

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

Assignees

No one assigned

Labels

area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 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.

Typescript: fields defined with definite assignment assertion are removed with TS compiler flag useDefineForClassFields set

4 participants

@akphi @babel-bot @nicolo-ribaudo @existentialism