babel: v7 plugin-proposal-class-properties transform flow type annotation to undefined

Bug Report

Current Behavior

@babel/plugin-proposal-class-properties transform flow type annotation to a property assigning undefined.

this may caught statement order unexpected error.

Input Code

let code = `
class B {}
class Logger extends B {
  log: {
    error: boolean
  }
  silent: boolean
  log = {
    error: true
  }
  get silent(): boolean {
    return this._silent;
  }
  set silent(val: boolean) {
    this._silent = !!val;
    if (!this.log) {
      throw new Error('#log should not be nil.')
    }
    Object.keys(this.log).forEach(k => { /* do something... */  })
  }
  constructor() {
    super()
  }
}
`

let r = require("@babel/core").transform(code, {
  "presets": [ "@babel/preset-flow" ],
  "plugins": [
    [ "@babel/plugin-proposal-class-properties", { "loose": true } ]
  ]
})
console.log(r.code)

ouput =>

class B {}

class Logger extends B {
  get silent() {
    return this._silent;
  }

  set silent(val) {
    this._silent = !!val;

    if (!this.log) {
      throw new Error('#log should not be nil.')   // <----- will throw if the annotation property assigning undefined
    }

    Object.keys(this.log).forEach(k => { /* do something... */ });
    });
  }

  constructor() {
    super();
    this.log = void 0;  <<<<<----
    this.silent = void 0; <<<<<---- with setter defined.
    this.log = {
      error: true
    };
  }
}

Expected behavior/code

class B {}

class Logger extends B {
  get silent() {
    return this._silent;
  }

  set silent(val) {
    this._silent = !!val;

    if (!this.log) {
      throw new Error('#log should not be nil.')
    }

    Object.keys(this.log).forEach(k => { /* do something... */ });
  }

  constructor() {
    super();
    this.log = {
      error: true
    };
  }

}

Environment

  • Babel version(s): [e.g. v6.0.0, v7.0.0-beta.55]
  • Node/npm version: [e.g. Node 8/npm 5]
  • OS: [e.g. OSX 10.13.4]
  • Monorepo [e.g. yes/no/Lerna]
  • How you are using Babel: js

Possible Solution

Should strip the ClassProperty type of TypeAnnotation.

About this issue

  • Original URL
  • State: closed
  • Created 6 years ago
  • Reactions: 26
  • Comments: 25 (8 by maintainers)

Commits related to this issue

Most upvoted comments

So, as much as this is personally annoying, b/c our existing codebase was written based on the existing Flow transform (i.e., no initial undefined value), it does appear the current babel@7 treatment is spec compliant (emphasis added):

  1. If initializer is not empty, then a. Let initValue be ? Call(initializer, receiver).
  2. Else, let initValue be undefined.

This sort of potential conflict comes with the territory of pre-spec transpilation and supra/meta-language constructs (i.e., Flow).

However, it raises the issue that there is, at least FWICT, now no suitable means to add a class property flow annotation except through a flow comment (which I would prefer to avoid if possible)… The conflict is unfortunate, but I don’t think it’s reasonable to break from language compliance in favor of flow compliance. Flow will necessarily have to evolve/change to address this edge case.

Best Recommendation for those in my/this situation is @lxcid’s recommendation to ensure @babel/plugin-transform-flow-strip-types runs (removing foo: bar;) prior to @babel/plugin-proposal-class-properties in the plugin ordering. You’ll effectively be writing spec non-compliant code, but I assume Flow will address the issue with an easy/simple and hopefully automatic upgrade/codemod in the near future.

See other discussion here: #8762 https://github.com/babel/babel/pull/8584 https://github.com/facebook/flow/issues/6811

I came across this issue using react-native 0.56 and babel v7.0.0-beta.47 as well. In my case, an instance property gets overwritten with void 0 after already being set by the base class constructor. While I’m not sure that this is the best solution for solving this issue, I fixed the issue for my case by adding a line from the 6.x branch that skips initialisation for all unset instance properties. Here is the patch I used for beta.47:

--- a/node_modules/@babel/plugin-proposal-class-properties/lib/index.js
+++ b/node_modules/@babel/plugin-proposal-class-properties/lib/index.js
@@ -197,6 +197,7 @@ var _default = (0, _helperPluginUtils().declare)((api, options) => {
           if (propNode.static) {
             staticNodes.push(buildClassProperty(ref, propNode, path.scope, state));
           } else {
+            if (!propNode.value) continue;
             instanceBody.push(buildClassProperty(_core().types.thisExpression(), propNode, path.scope, state));
           }
         }

I’m having this issue too while running jest, in particular with example files like: https://github.com/facebook/react-native/blob/master/Libraries/Animated/src/createAnimatedComponent.js, in where type definition and declaration is being made with flow. If you have a look at the file pointed above, you will find:

class AnimatedComponent extends React.Component<Object> {
    // ...
    _component: any;
    _invokeAnimatedPropsCallbackOnMount: boolean = false;
    _prevComponent: any;
    _propsAnimated: AnimatedProps;
    _eventDetachers: Array<Function> = [];
    _setComponentRef: Function;
    // ...
    constructor(props: Object) {
        super(props);
        this._setComponentRef = this._setComponentRef.bind(this);
    }
    // ...
    _setComponentRef(c) {
        this._prevComponent = this._component;
        this._component = c;
    }
    // ...
}

As you can see, some of the property type definitions have default values too, as for example _invokeAnimatedPropsCallbackOnMount, but, lets focus on _setComponentRef, which is a function. If you keep looking at the code below, in particular the class constructor() method:

constructor(props: Object) {
   super(props);
   this._setComponentRef = this._setComponentRef.bind(this);
}

The method _setComponentRef is being defined later on AnimatedComponent source code, but, at the time this file gets compiled by babel, and @babel/preset-flow is applied, _setComponentRef gets initialized with void 0, it does’t matter loose: true or loose: false, in the end its value will be void 0. Having that in mind, at the time this._setComponentRef = this._setComponentRef.bind(this); runs, this._setComponentRef has been set with void 0, which causes:

TypeError: Cannot read property 'bind' of undefined

      at new bind (node_modules/react-native/Libraries/Animated/src/createAnimatedComponent.js:39:53)
      at constructClassInstance (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:3530:18)
      at updateClassComponent (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:5227:7)
      at beginWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:5907:14)
      at performUnitOfWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:7949:12)
      at workLoop (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:7980:24)
      at renderRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8020:7)
      at performWorkOnRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8592:22)
      at performWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8527:7)
      at performSyncWork (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:8499:3)

It seems something in the process is ignoring _setComponentRef definition later in the code, and only stays with _setComponentRef: Function, which is indeed translated into _setComponentRef = void 0 because it does not specify any default value, but in the end this is not correct, because it should have been initialized to the method definition _setComponentRef(c) { ... } that is located later in the code, as displayed above.

If I remove _setComponentRef: Function; at the top of AnimatedComponent the error disappears, because _setComponentRef gets initialized as expected, pointing to the method defined later in the code.

Any thoughts?

Thanks.

Luckily with Flow there is a third solution but not with TypeScript 😦

// The solution is Flow comments
class A extends Record({ x: number }) {
  /*:: x: number; */ // This will be type checked by Flow but doesn't emit code since it's a comment
}

@aliceriot Would you be able to do

class Editor extends React.Component<Props, State> {
  dispatchTransaction: Function = this.dispatchTransaction.bind(this);
  
  constructor(props: Props) {
    super(props);
  }
}

to sidestep the issue?

I wonder if that behavior was intentionally left out, but based on the comments I would assume that this is not spec compliant and a regression.

yes, I’m also prefer to strip these annotations without initial value. below diffs is my localized fixes:

diff --git a/packages/babel-plugin-proposal-class-properties/src/index.js b/packages/babel-plugin-proposal-class-properties/src/index.js
index 3c333fdd3..30a0dd6a1 100644
--- a/packages/babel-plugin-proposal-class-properties/src/index.js
+++ b/packages/babel-plugin-proposal-class-properties/src/index.js
@@ -366,6 +366,9 @@ export default declare((api, options) => {

         let p = 0;
         for (const prop of props) {
+          // Check property is an type annotation, and skip it
+          if (prop.has("typeAnnotation")) continue;
+
           if (prop.node.static) {
             staticNodes.push(buildClassProperty(t.cloneNode(ref), prop, state));
           } else if (prop.isPrivate()) {

This is fixed by https://github.com/babel/babel/pull/11178, which allows you to use declare in front of class fields to mark then as type-only and thus don’t initialize them to undefined.

Wow, I just literally spent 5 hours figuring out why my react-native iOS app was not doing any network request. It just didn’t resolve or reject. Kudo’s for the people here mentioning that @babel/plugin-proposal-class-properties is the dealbreaker.

@loganfsmyth It’s not possible. because Immutable.Record create a class that initialize all fields as getters and setters when a setter throws.

  1. In normal mode it will override the functionality (Not sure how it will affect Record#asMutable()) . (And lets say it’s okay, it will break up mobx ☹️)
  2. In loose mode it will throw

But thanks, I’ll use the first option for now. But I think in loose mode it would be better when there are only type annotations, it will just strip them instead of doing this.x = void 0