Closed Bug 1705503 Opened 4 years ago Closed 4 years ago

Update DevTools Babel builds to consistently use proposed class property semantics for no-initializer props

Categories

(DevTools :: General, task, P3)

task

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1575105

People

(Reporter: heinleinjenn88, Unassigned, Mentored)

Details

+++ This bug was initially created as a clone of Bug #1575105 +++

Tasks

  1. Convert the 3 instances where the devtools Babel config lists flow-strip-types followed by class-properties, to instead swap the two around so that class-properties is before flow-strip-types.
  2. Review the places in the code where things start failing, and see what we can do to fix them.

In a perfect world, this will mostly involve removing class properties entirely, since we don't need them, or updating the code to have an initializer value. For instance, given the examples farther down, onClick: Function could either be deleted, or changed to onClick: Function = this.onClick; so that the value continues to pass through like it did before.

Context

Babel 6 implemented the class property specification as it was defined at the time, e.g.

class Foo {
  prop;
}

would be treated the same as

class Foo {}

However as of Babel 7.x and newer versions of the spec, this is in fact equivalent to

class Foo {
  prop = undefined;
}

This only really has issues if you declare a type as a class property, but then it also exists on the prototype chain. With the behavior I described here, you're essentially doing this.prop = undefined, so if prop was used later as a prototype function, things could fail because it is now unexpectedly overwritten.
This so far seems to be totally fine with our existing codebase, but that is because we are currently protected by somewhat of an accident.

Issues arise when you take Flowtype into account. With Flow for instance, it is common to do

class Foo2 {
  onClick: Function;

  constructor() {
    this.onClick = this.onClick.bind(this);
  }

  onClick() {}
}

This is a problem, because it essentially does

this.onClick = undefined;
this.onClick = this.onClick.bind(this);

causing an exception.

Why isn't this an issue right now? Because Babel has a semi-accidental workaround that preserves the behavior from Babel 6 specifically when a class property has a type annotation on it and no initializer value. Surprise! This specifically saves us because our config does

        "@babel/plugin-transform-flow-strip-types",
        "@babel/plugin-proposal-class-properties",

in that order, with Flow types stripped first. This means that the onClick: Function; is actually deleted before Babel gets the chance to convert it into this.onClick = undefined;.

This technically means that, in the case where we have a type annotation, the class property is not following the specification behavior.

Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.