Fix type annotation properties assigning to undefined. (#8417)

[allex]

Q	A
Fixed Issues?	Fixes #8417
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	Add a helper declareNilProps (obj, props); to @babel/babel-helpers
License	MIT

[babel-bot]

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

[nicolo-ribaudo]

This should be 7.0.1, since it represents the first version where the helper is available.

[nicolo-ribaudo]

I don't undertand why the _declareNilProps helper is needed. Why should x: number create a property on the class if it doesn't have an initializer? Isn't it just a type annotation?

[loganfsmyth]

I don't see us landing this without a serious discussion with the folks developing the class fields specification and Flow folks. What you're proposing means that if you have

class Foo {
  x;
  y;
}

console.log("x" in new Foo());

in an existing codebase, and you want to convert the codebase to Flow by doing

class Foo {
  x: number;
  y: number;
}

console.log("x" in new Foo());

the behavior of the code would change based on the addition of a type annotation, which seems like it would be incredibly surprising.

[nicolo-ribaudo]

@loganfsmyth We already remove x: foo; if the flow plugin runs before the class-properties one. I think that we should fix this inconsistency.

FWIW, I opened facebook/flow#6811 yesterday but it isn't getting much attention from the flow team 😕

[loganfsmyth]

@nicolo-ribaudo Yeah, the inconsistency is unfortunate, but I think changing it at this stage would be even more confusing. Maybe once we've had all the discussions with Flow and spec folks we can revisit.

[c0d3m3nt0r]

@loganfsmyth, that scenario you mention makes sense, I mean, if you declared two class variables, and then you do not initialize them, they will remain undefined, which is expected, I think, at least I wouldn't expect another behavior in that case.

The thing gets inconsistent, at least from my point of view, when after declaring a variable, like in your example, but having in mind that variable is an arrow function, when it later gets initialized in the code, at the time the constructor gets executed, that variable remains undefined, like in react-native createAnimatedComponent() source code, and as I explained here, there, is in where things turn out to be inconsistent.

In my opinion, I believe this is related to the part in where class properties are being processed, I would have expected @babel/proposal-class-properties took care of these kind of use cases and output the correct class properties initialization, in where (in the case I pointed) the variable should be other than undefined.

The easiest path would be, in my case, to make react-native team to consider removing the Flow type annotation for the arrow function at the top of their class, and just initialize the arrow function as they do later in their code (and at the same time make it more accurate, in terms of parameter types and return type), and that should fix the issue, but, the problem is all other libraries that are doing the same thing, having in mind this issue is not happening with Babel 6.x.

So, having that in mind, how can we solve this situation?, should we ask everyone to adjust their code in order to workaround this issue? (besides we agree or not that one is a good practice or makes sense or not) or to find a backward compatible solution to make current libraries having the same issue to have their pipeline back to work?

Sooner or later someone will fix this, but in the meantime, the code starts to get full or workarounds to get rid of these kind of issues when moving to Babel 7. I have upgraded to the latest stable version of babel (7.0.0), and I have to recognize several issues were fixed since the beta.47, which is a great job to address those issues to finally arrive to something more stable, but this one it is still there, and it would be great to have it fixed at some point :)

Thank you for your support and effort to keep making Babel such a great tool.

[c0d3m3nt0r]

@allex, would you mind to try something like the following?

class A {
   fn: Function;
   constructor() {
      this.fn = this.fn.bind(this);
   }
   fn = () => {
      return 'hey!';
   }
}

I'm interested on determine whether the constructor will throw TypeError: Cannot read property 'bind' of undefined (due to fn gets initialized to undefined) or the code runs successfully having resolved fn to the actual arrow function value defined later in the example.

[allex]

Yes, it's should be ok, seem like:

  class A {
    fn: Function;
    constructor() {
      this.fn = this.fn.bind(this);
    }
    fn = () => {
      return 'hey!';
    }
  }

  const a = new A();
  expect(a.fn()).toBe('hey!')

[c0d3m3nt0r]

Cool, thanks 👍

[nicolo-ribaudo]

I am ok with mergint this, since it what we alread do when the flow plugin runs before the class-properties one.
Also, its what flow currently does:

➜ npx flow-remove-types
npx: installed 3 in 0.757s
> class A {
... constructor() { this.x = 2 }
... }
undefined
> class B extends A {
... x: number;
... }
undefined
> new B().x
2
> 

If the flow teams decides that this behavior is wrong, we can change it in the future.

[torkelo]

Any update to get this merged? This is causing issue wtih typescript + babel

[nicolo-ribaudo]

I'm closing this PR because you can already use declare for class fields with TypeScript to avoid initializing them to undefined, and starting from Babel 7.9.0 it will also be supported by Flow (#11178).

Thanks @allex anyway!