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: always strip declare from class fields #11747

Merged

Conversation

@jamescdavis
Copy link
Contributor

jamescdavis commented Jun 25, 2020

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

Normally, a class field annotated with declare would be stripped because declare indicates this is type-only information. The exception is when a decorator is also applied to the property, e.g.

class Foo {
  @bar declare x: string;
}

In this case, the property should be preserved so that the decorator can do its thing. Currently babel-plugin-transform-typescript is correctly not stripping the property, but it is also not stripping the declare.

This PR fixes this by always removing declare if still found if the path has not been removed.

This matches the behavior of the TypeScript compiler: https://www.typescriptlang.org/play/?experimentalDecorators=true#code/MYGwhgzhAEBiD29oG8CwAoa0ACATApsPAE5gAuJ0BoYx+0AZogFzQRnECWAdgOYDcGLNXB1oAI1qt2XPoPQBfIA

I've pushed a failing test first to demonstrate the issue and then pushed the fix.

@codesandbox
Copy link

codesandbox bot commented Jun 25, 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 6a82099:

Sandbox Source
stoic-satoshi-1z2fl Configuration
blissful-ives-80mqi Configuration
@babel-bot
Copy link
Collaborator

babel-bot commented Jun 25, 2020

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

Copy link
Contributor

JLHwung left a comment

Thanks.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jun 26, 2020

@DanielRosenwasser Was making declare a noop with decorators a purposeful decision, or is it an unexpected side effect of how decorators are handled?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 26, 2020

@sandersn and @rbuckton can better answer this question.

@chriskrycho
Copy link

chriskrycho commented Jun 26, 2020

At least from the perspective of the Ember world, it's extremely helpful. Ember makes heavy use of dependency injection using decorators, like so:

import Component from '@glimmer/component';
import { inject as service } from '@ember/service';
import type Session from 'my-app/services/session';

export default class Profile extends Component {
  @service declare session: Session;
}

The TS implementation gives us all the desired semantics here: the item can be declared for decoration, but because it has declare it doesn't trigger the requirement to mark with the definite initialization ! operator that it otherwise requires, and it actually correctly captures the semantics here: we declare that this will exist, will not be optional, and then let the decorator do its thing.

@rbuckton
Copy link

rbuckton commented Jun 26, 2020

Is there any concern with leaving the property in and how that would interact with other transforms for class fields? For example, in TS the declare modifier prevents any emit for the class field in the constructor when the --useDefineForClassFields flag is enabled.

@jamescdavis
Copy link
Contributor Author

jamescdavis commented Jun 27, 2020

@rbuckton doesn't declare prevent emit in the constructor even without useDefineForClassFields?: https://www.typescriptlang.org/play/#code/MYGwhgzhAEBiD29oG8CwAoa0AmBTUYATrtAEZEBc0EALoQJYB2A5gNwYC+QA

But a decorated declare always ends up decorated (even with useDefineForClassFields): https://www.typescriptlang.org/play/?useDefineForClassFields=true&experimentalDecorators=true#code/MYGwhgzhAEBiD29oG8CwAoa0ACATApsPAE5gAuJ0BoYx+0ARrQFzQRnECWAdgOYDcGAL5A

If plugin-typescript-transform leaves the property in and decorated and just strips declare, the decorators plugin will handle the rest.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jun 27, 2020

I have played a bit with TS's playground and even if this doesn't match TS's behavior* it's probably a good compromise.

* Babel's decorators and TypeScript's decorators have two completely different implementations. TypeScript applies fields decorators on the prototype, while Babel applies them on the instance. This means that, for example, this code has two different behaviors:

class A {
  x = 1;
}
class B extends A {
  @nonconfigurable declare x: number;
}

With TS, new B() has a non-configurable property set to 1, while Babel has a non-configurable property set to undefined.
Without this PR, the behaviors are also different: new B() has a configurable property set to 1.

The longer-term solution (Babel 8?) would be to publish a new @babel/plugin-transform-typescript-decorators, which transpiles decorators exactly like TS does:

  • This PR should be then reverted
  • If we see @decorator declare x with the "normal" decorators plugin, it should throw
  • @decorator declare x should finally match TS.

Can you add this test?

class Foo {
  @decorator declare bar: string;
}
"transform-typescript" { allowDeclareFields: true }
"proposal-decorators" { legacy: true }
"proposal-class-properties"
@jamescdavis
Copy link
Contributor Author

jamescdavis commented Jun 30, 2020

@nicolo-ribaudo test added

Copy link
Member

nicolo-ribaudo left a comment

I want to restate that I don't like this PR 😛

However, I think that it's the best we can have for now.

@chriskrycho
Copy link

chriskrycho commented Jul 7, 2020

FWIW, I think we’ll all be happier once it’s nicely resolved, assuming someday decorators are nicely resolved. 😜

@JLHwung JLHwung merged commit aa82ab6 into babel:main Jul 30, 2020
8 of 9 checks passed
8 of 9 checks passed
build
Details
test262 Workflow: test262
Details
Gitpod Open an online workspace in Gitpod
Details
Travis CI - Pull Request Build Passed
Details
babel/repl REPL preview is available
Details
build-standalone Workflow: build-standalone
Details
ci/codesandbox Building packages succeeded.
Details
codecov/project 91.85% (target 90.00%)
Details
e2e Workflow: e2e
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.