Fix type annotation properties assigning to undefined. (#8417) by allex · Pull Request #8584 · babel/babel

allex · 2018-08-30T06:41:34Z

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 · 2018-08-30T06:50:35Z

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

nicolo-ribaudo · 2018-08-30T07:08:21Z

packages/babel-helpers/src/helpers.js

  }
 `;
+
+helpers.declareNilProps = helper("7.0.0-beta.0")`


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

nicolo-ribaudo · 2018-08-30T07:11:51Z

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 · 2018-08-30T16:40:53Z

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 · 2018-08-30T17:23:12Z

@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 · 2018-08-30T17:25:20Z

@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 · 2018-08-30T20:07:52Z

@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 · 2018-08-30T20:21:25Z

...abel-plugin-proposal-class-properties/test/fixtures/public/type-annotation-transform/exec.js

+    ver: number
+    ver = x;
+
+    fn: Function


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

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!')

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 · 2018-11-23T10:52:05Z

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

nicolo-ribaudo · 2020-03-17T09:34:41Z

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!

nicolo-ribaudo reviewed Aug 30, 2018

View reviewed changes

allex force-pushed the babel-plugin-proposal-class-properties branch 2 times, most recently from c16a4c3 to 06f7846 Compare August 30, 2018 09:48

c0d3m3nt0r reviewed Aug 30, 2018

View reviewed changes

Fix type annotation undefine, strip no initial properties. (babel#8417)

a341c06

allex force-pushed the babel-plugin-proposal-class-properties branch from 37e660b to a341c06 Compare August 31, 2018 05:31

nicolo-ribaudo approved these changes Sep 5, 2018

View reviewed changes

rtsao mentioned this pull request Sep 14, 2018

Fix Flow-annotated class properties/methods fusionjs/fusion-cli#516

Merged

CrabDude mentioned this pull request Oct 15, 2018

v7 plugin-proposal-class-properties transform flow type annotation to undefined #8417

Closed

nicolo-ribaudo mentioned this pull request Dec 7, 2018

Add "ignoreUninitialized" option to "@babel/proposal-class-properties" #9141

Closed

nicolo-ribaudo closed this Mar 17, 2020

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 17, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jun 17, 2020

Uh oh!

Conversation

allex commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Aug 30, 2018

Uh oh!

loganfsmyth commented Aug 30, 2018

Uh oh!

nicolo-ribaudo commented Aug 30, 2018

Uh oh!

loganfsmyth commented Aug 30, 2018

Uh oh!

c0d3m3nt0r commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c0d3m3nt0r Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allex Aug 31, 2018

Choose a reason for hiding this comment

Uh oh!

c0d3m3nt0r Aug 31, 2018

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torkelo commented Nov 23, 2018

Uh oh!

nicolo-ribaudo commented Mar 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

allex commented Aug 30, 2018 •

edited

Loading

babel-bot commented Aug 30, 2018 •

edited

Loading

c0d3m3nt0r commented Aug 30, 2018 •

edited

Loading

c0d3m3nt0r Aug 30, 2018 •

edited

Loading

nicolo-ribaudo left a comment •

edited

Loading