Typescript: always strip declare from class fields

[jamescdavis]

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-ci]

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]

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

[JLHwung]

Thanks.

[nicolo-ribaudo]

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

[DanielRosenwasser]

@sandersn and @rbuckton can better answer this question.

[chriskrycho]

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]

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]

@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]

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]

@nicolo-ribaudo test added

[nicolo-ribaudo]

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]

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