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

Typescript: Avoid stripping class properties when a decorator is set #8238

Merged
merged 1 commit into from Jul 4, 2018

Conversation

@pmdartus
Copy link
Contributor

pmdartus commented Jun 29, 2018

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

Current behavior:
The babel-plugin-transform-typescript removes class properties without value regardless if decorators are assigned to it or not.

// Original
class C {
  @foo
  d;
  @foo
  e = 3;
}

// Transformed
class C {
  // <-- Missing `d` field
  @foo
  e = 3;
}

Expected behavior:
The babel-plugin-transform-typescript should only remove a class property if it has no value and no decorators.

// Original
class C {
  @foo
  d;
  @foo
  e = 3;
}

// Transformed
class C {
  @foo
  d;
  @foo
  e = 3;
}

REPL example

@@ -157,10 +157,6 @@ export default declare((api, { jsxPragma = "React" }) => {

ClassProperty(path) {
const { node } = path;
if (!node.value) {

This comment has been minimized.

Copy link
@pmdartus

pmdartus Jun 29, 2018

Author Contributor

This code is a duplicate of the one in the Class visitor.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Jun 29, 2018

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

@pmdartus pmdartus changed the title Avoid stripping class properties when a decorator is set Typescript: Avoid stripping class properties when a decorator is set Jul 1, 2018
@diervo
diervo approved these changes Jul 2, 2018
@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Jul 4, 2018

I don't have enough context here, sorry. Why do we want to emit decorators syntax? Shoudln't we strip out unsupported syntax?

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Jul 4, 2018

The typescript plugin should leave decorators there: they will be handled by the decorators plugin.

@diervo

This comment has been minimized.

Copy link
Contributor

diervo commented Jul 4, 2018

@nicolo-ribaudo @xtuc are you good with this changes?
The idea is for typescript to not touch any feature that will need further transform (in particular decorators and class fields - which they need to update anyway in their transpiler)

If so, shall bribe @hzoo or @loganfsmyth to merge this? :)

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Jul 4, 2018

I'm 👍 only if we can make sure the user is aware of this behavior:

  • we should warn if decorator are not transformed (because no plugin loaded)
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Jul 4, 2018

I don't think that we should, the same way we don't warn if class properties or other proposals supported by typescript are not transpiled.
Users will see very easily that their code is not transformed, since it would cerash at the line containing the decorators as soon as it is loaded in a browser.

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Jul 4, 2018

That's right @nicolo-ribaudo, I was wondering if the "why it didn't transform it" would be confusing for users or not.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Jul 4, 2018

If the user just adds the typescript plugin, Babylon throws and suggests the user to add the transform-decorators plugin.
The only problem would be if the users only enables the syntax plugin, but it is known that syntax plugins leave the input as-is.

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Jul 4, 2018

This is fine, it's expected behavior 👍.

@hzoo hzoo merged commit 4d125c3 into babel:master Jul 4, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.75% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Jul 4, 2018

Ah yes Nicolo, perfect!

@lock lock bot added the outdated label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.