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.

hello, I would like to take this up.

@Ruchika Totally. Once you get started, I'd be happy to talk with you on Slack.

thankyou @loganfsmyth, working on this one.
Also, just checked and found the following file:

mozilla-central/devtools/client/debugger/babel.config.js

Hope this is the one to be changed?

We'll need to update these:

• https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/devtools/client/shared/build/build-debugger.js#265
• https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/devtools/client/shared/build/build.js#13
• https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/devtools/client/debugger/babel.config.js#33

Hey, I've done the required changes for the other files but I cannot find this one: https://searchfox.org/mozilla-central/source/devtools/client/debugger/babel.config.js

hello @loganfsmyth, this bug doesn't shoe up in my assigned list as of now. Could you please assign it to me? :)

Hi Ruchika, I see this bug has not changed in the past month (nor the review on phabricator). Are you still interested in working on this? If you need more time, that's totally fine. Just let us know.

hi, yes im interested in continuing to work on this one. was busy with work and exams :)

I think it’s probably safe to unassign this bug and make it available for others.

Ruchika, please speak up if you are still interested.

Honza

Hey! Would love to pick this up if it’s still available

Julian, do you have some free cycles to help with review on this one?

Honza

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #14)

Julian, do you have some free cycles to help with review on this one?

Honza

Sure, I'll add myself as mentor.

(In reply to Ismail Shak from comment #13)

Hey! Would love to pick this up if it’s still available

Thanks! Assigning the bug to you.

Awesome. Will set everything up and get started. Is there a preferred way of reaching out with questions? (Hopefully won’t be bothering you that much haha)

(In reply to Ismail Shak from comment #16)

Awesome. Will set everything up and get started. Is there a preferred way of reaching out with questions? (Hopefully won’t be bothering you that much haha)

Sorry, forgot to reply here!
One of the most consistent ways is to use the "request information from" field in bugzilla (below the comment box).

Sorry, I got caught up in work and didn't start diving in 'till today. I understand what needs to be done (mostly) and had a few Qs around orienting myself in the code base and just small general clarification questions

You can also find us on Matrix if you have more general question about the codebase/setup: https://chat.mozilla.org/#/group/+devtools-team:mozilla.org

Otherwise, usually we use the "needinfo" feature when asking a specific question and I don't see one in your comment above, so I assume you would prefer to chat.

I realize this is really late and I apologize for the disappearance, I had a lot going on. I won't be able to continue contributing unfortunately, so wanted to let you know so you can take me off this ticket and open it up for others. Best, Ismail.

Hello, could I get a shot at this bug please?

(In reply to Manuel Carretero from comment #21)

Hello, could I get a shot at this bug please?

Sure!
Just let me check first if the bug is still relevant, because we changed a lot of things around the Debugger build steps in the past months.

We removed most flow usage in the debugger in Bug 1690742 (landed 2 months ago).
At the same time we also got rid of most transform-flow-strip-types, so this bug no longer applies.

We left a few occurrences of transform-flow-strip-types in the codebase though, https://searchfox.org/mozilla-central/search?q=transform-flow-strip-types&path=devtools%2Fclient%2Fdebugger%2Fpackages

I will repurpose this bug to cleanup those remaining occurrences (simply remove them and check that nothing breaks on CI).
Would you be interested, should I assign it to you?

Sure, I'm still interested

Great, assigning the bug to you!

From what I can see, we should remove everything listed on https://searchfox.org/mozilla-central/search?q=transform-flow-strip-types&path=devtools%2Fclient%2Fdebugger%2Fpackages

There are other mentions of flow-strip-types in the debugger tests, but I wouldn't touch those.

https://hg.mozilla.org/mozilla-central/rev/78764895b2a6