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] Typescript plugin overwrites parent class properties when the child narrows that property's type #9105

Closed
arilotter opened this issue Nov 30, 2018 · 8 comments
Labels
area: typescript area: upstream awaiting reply outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Comments

@arilotter
Copy link

Bug Report

Current Behavior
When a class (B) extends another class (A) and includes a type definition to narrow the type of something specified in class A, it will set the narrowed property's value to undefined. This diverges from the TSC behavior.
The output from the below code under babel is a, undefined.
Input Code

  class A {
    value: any
    constructor(value: any) {
      this.value = value
    }
  }

  class B extends A {
    value: string
  }

document.write(`${new A('a').value}, ${new B('b').value}`)

Expected behavior/code
The output from the code should be a, b.

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

.babelrc:

{
  "presets": [
    "@babel/env",
    "@babel/typescript"
  ],
  "plugins": [
    "@babel/proposal-class-properties",
    "@babel/proposal-object-rest-spread"
  ]
}

Environment

  • Babel version(s): 7.1.6
  • Node/npm version: Node 10 / NPM 6
  • OS: Any OS
  • Monorepo: doesn't matter
  • How you are using Babel: loader

Possible Solution
It seems like babel is setting value = undefined in the class property - it needs to check if that value is defined in the parent before overwriting it.

@babel-bot
Copy link
Collaborator

Hey @arilotter! 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.

@ryami333
Copy link

ryami333 commented Dec 3, 2018

This issue seems to be blocking a tsc->@babel/preset-typescript migration we're trying to perform. In our case, we're extending Immutable's Record like this:

import { Record } from 'immutable';

const defaults = { legCount: 2 };
class Animal extends Record(defaults) {
   legCount: number;
}

const cat = new Animal({ legCount: 4 });

console.log(cat.legCount);

// Expect: 4
// Actual: undefined

We have the same babel config as the OP. I've tried configuring it explicitly with @babel/plugin-transform-typescript (rather than the preset) so that I can force it's priority higher than @babel/proposal-class-properties but it didn't seem to help. From memory it just tripped up entirely on the class attribute syntax.

@loganfsmyth
Copy link
Member

This is expected behavior with Babel, because Typescript does not conform to the class field proposal's behavior and Babel does. If a parent class is responsible for defining a type, you should not be re-declaring it on the child class, because that means you are explicitly taking responsibility for making sure the value is correct.

You'd either need to do legCount: number = this.legCount; to explicitly reuse the value defined by the parent class, or leave it out entirely because it can already infer the type of legCount from the argument to Record.

@buschtoens
Copy link
Contributor

buschtoens commented Dec 3, 2018

I've opened microsoft/TypeScript#28823 to raise awareness about this issue. It was marked as a duplicate of microsoft/TypeScript#12437.

While I really appreciate that Babel is pretty much 100 % spec compliant, I also would like it to be able to process TypeScript files correctly. I don't have high hopes for TS fixing the issue upstream, so I think a solution in Babel-land would be in order.

If the @babel/plugin-transform-typescript is run before @babel/plugin-proposal-class-properties, it works, because the empty declarations are just stripped. However, this then breaks this subtle behavior:

class TestClass {
  // we shouldn't encourage folks to write code like this, but tsc ensures
  // that constructor param fields are set before field initializers run
  field = this.constructorParam;
  constructor(private constructorParam: string) {}
}

let instance = new TestClass('hello');
assert.equal(instance.field, 'hello');

From ember-cli-typescript.

So not matter how you order the two plugins, one or the other thing breaks.

Ideas for possible solutions:

  • Optional third transform to be run before @babel/plugin-proposal-class-properties that strips ClassProperty without a value.
  • Make @babel/plugin-proposal-class-properties configurable to strip ClassProperty without a value.
  • Inline @babel/plugin-proposal-class-properties into @babel/plugin-transform-typescript and ensure correct behavior through some clever changes.

@nicolo-ribaudo
Copy link
Member

Thank you! Lets see what the TS team says.

@galdebert
Copy link

galdebert commented May 30, 2019

One solution worth noting that I used to migrate from tsc to babel is declaration merging:

  interface A {}
  interface B {}

  class Base {
    prop: A | B | null = null;
  }

  class Sub1 extends Base {
    prop!: A | null; // type narrowed
  }

  // using declaration merging
  interface Sub2 {
    prop: A | null; // type narrowed
  }
  class Sub2 extends Base {
  }

  console.log(new Sub1().prop); // tsc: null, babel: undefined
  console.log(new Sub2().prop); // tsc: null, babel: null

@hangoocn
Copy link

hangoocn commented Aug 12, 2019

Thanks for @galdebert 's idea, I have done the same thing using Declaration Merging:
Issue:

class Base {
   constructor(value: string) {
       this._value = value;
   }
    _value: string;
}

class A extends Base {
    constructor(value: string) {
        super(value;)
    }

    _value: string;
    public get value() {
        return this._value;
    }
}
const a = new A("abc");
console.log(a.value); // undefined

But if we use the declaration merging like this:
Solution:

class Base {
   constructor(value: string) {
       this._value = value;
   }
    _value: string;
}

interface A {
    _value: string;
}

class A extends Base {
    constructor(value: string) {
        super(value;)
    }

    public get value() {
        return this._value;
    }

}
const a = new A("abc");
console.log(a.value); // "abc"

@nicolo-ribaudo
Copy link
Member

You can now use declare with class fields (TS 3.7, Babel 7.7)

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 17, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: typescript area: upstream awaiting reply outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants