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

Fix TSParameterProperty getting lost with transform-classes #8682

Merged
merged 3 commits into from
Sep 14, 2018

Conversation

existentialism
Copy link
Member

@existentialism existentialism commented Sep 11, 2018

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

@existentialism existentialism added PR: Bug Fix 🐛 A type of pull request used for our changelog categories area: typescript labels Sep 11, 2018
@babel-bot
Copy link
Collaborator

babel-bot commented Sep 11, 2018

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

@@ -190,10 +144,63 @@ export default declare((api, { jsxPragma = "React" }) => {
// We do this here instead of in a `ClassProperty` visitor because the class transform
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this comment is slightly out of date, now that this block contains both ClassProperty and ClassMethod logic.

param.type === "TSParameterProperty" &&
!PARSED_PARAMS.has(param.parameter)
) {
PARSED_PARAMS.add(param.parameter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly out of curiosity - it looks like this is the only real functional change added to this block, to prevent duplicate parameters from being added to the parameterProperties list? What sort of case is this preventing? How would two different entries of childNode.params have the same parameter object?

Copy link
Member Author

@existentialism existentialism Sep 11, 2018

Choose a reason for hiding this comment

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

It requires a bit of understanding of the class transformation:
https://github.com/babel/babel/blob/master/packages/babel-plugin-transform-classes/src/index.js#L41

Ultimately, it prevents params from getting parsed twice since the class transform converts it to an expression.

@Retsam
Copy link
Contributor

Retsam commented Sep 11, 2018

Thanks for fixing this!

I believe this fix still depends on transform-typescript running before transform-classes, right? I don't see any way to avoid that, but do we know if that caveat is documented anywhere? If not, it'd be nice to have some warning about that, somewhere.

@nstepien
Copy link
Contributor

@existentialism Yo, just wanted to let you know about a related issue that this PR doesn't seem to fix, judging from the repl build results: #8692

child.node.typeAnnotation = null;
const childNode = child.node;

if (t.isClassMethod(childNode) && childNode.kind === "constructor") {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you can use t.isClassMethod(childNode, { kind: "constructor" })

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
6 participants