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 definite assignment emit #10311

Open
matt-tingen opened this issue Aug 8, 2019 · 6 comments

Comments

@matt-tingen
Copy link
Contributor

commented Aug 8, 2019

Bug Report

Current Behavior
In loose mode, @babel/plugin-proposal-class-properties emits a property assignment for TypeScript definite assignments.

Input Code
REPL (I'm not sure if it's possible to turn turn on loose mode for the plugin)
TS Playground

// Like Object.assign but will not assign properties which are already defined
declare const assignDefaults: typeof Object.assign

class Foo {
  bar!: number;

  constructor(overrides: { bar: number }) {
    assignDefaults(this, overrides);
  }
}

Expected behavior/code
A definite assignment should not emit any JS.

// Like Object.assign but will not assign properties which are already defined
class Foo {
  constructor(overrides) {
    assignDefaults(this, overrides);
  }

}

Babel Configuration (.babelrc, package.json, cli command)

{
  "presets": ["@babel/preset-typescript"],
  "plugins": [["@babel/plugin-proposal-class-properties", { "loose": true }]]
}

Environment

  • Babel version(s): 7.5.5
  • Node/npm version: Node 12.6 / yarn 1.15.2
  • OS: macOS 10.14.5
  • Monorepo: no
  • How you are using Babel: cli

Additional context/Screenshots
This was previously raised in #7997, but considered expected behavior. It was mentioned that it could be reconsidered for loose mode, however.

I do feel like it should be reconsidered one way or another; emitting for these declarations is problematic in the same way emitting for a declare would be. Additionally, this syntax is not valid JS so there the only applicable spec compliance is that of TS.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2019

Hey @matt-tingen! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite.

@matt-tingen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

If anyone else runs into this, a workaround is to use declaration merging:

// Like Object.assign but will not assign properties which are already defined
declare const assignDefaults: typeof Object.assign

interface FooOverrides {
  bar: number;
}

interface Foo extends FooOverrides {}

class Foo {
  constructor(overrides: FooOverrides) {
    assignDefaults(this, overrides);
  }
}
@JLHwung

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

The TypeScript properties conflicts with tc39 Class Properties proposal. As Babel aligns with ECMAScript, so we would not fix it. In my opinion it should be fixed on TypeScript's side.

The workaround mentioned above is reasonable: simply declare fields in interface.

As this issue duplicates #7644 and it is not a bug that we will fix, I am gonna close it now.

@matt-tingen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

@JLHwung This is not a duplicate of #7644. That issue is asking about non-definite properties without an initializer e.g. class Foo { bar: unknown }. TypeScript does deviate from the proposal in that case, although it's basically a non-issue with strictPropertyInitialization.

In TypeScript, adding the ! makes it exclusively a type annotation similar to declare const foo: number. It's just an assertion to the compiler that something is assumed to happen in the constructor.

Skimming over the class properties spec, I don't see anything which makes class Foo { bar! } syntactically valid JavaScript.

For those reasons, I don't believe that this request conflicts with the proposal. However, if spec compliance is still a concern despite the ! difference, perhaps this could be implemented in loose mode as discussed in #7997.

@JLHwung

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

We are not trying to match TypeScript behaviors, the TypeScript parser plugin let you parse TypeScript and apply transformations. Actually behind the scene we model property members in TypeScript as a Class Property in ECMAScript.

It is a valid feature request for loose mode, where we expect less code size. I suggest we also implement #7997 together so that in loose mode a TypeScript property member should be stripped unless it has an initializer.

@JLHwung JLHwung reopened this Aug 12, 2019

@JLHwung JLHwung added i: enhancement and removed i: invalid labels Aug 12, 2019

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

I don't think that we need loose mode; we should just strip them in the TypeScript plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.