1458017 - Treeherder's UI does not work on ESR 52 due to react2angular ("TypeError: class constructors must be invoked with |new|")

Status: RESOLVED FIXED

(Tree Management :: Treeherder: Frontend, defect, P5)

Description

[Alice0775 White]

Build Identifire:
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build Id: 20180427222832
Firefox52.7.4ESR x86
Windows10 Home 64bit, Version 1709 buildOS16299.402




Browser console:
TypeError: class constructors must be invoked with |new|
Stack trace:
createInternalInjector/<.invoke@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:279524
$ControllerProvider/this.$get</</<@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:315173
nodeLinkFn@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:297868
compileNodes/<@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:293238
nodeLinkFn@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:298705
compileNodes/<@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:293238
compileNodes/<@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:293262
compileNodes/<@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:293262
nodeLinkFn@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:298705
compileNodes/<@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:293238
compileNodes/<@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:293262
compile/<@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:292019
bootstrap/vo/</<@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:266112
$RootScopeProvider/this.$get</Scope.prototype.$eval@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:354640
$RootScopeProvider/this.$get</Scope.prototype.$apply@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:354891
bootstrap/vo/<@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:266067
createInternalInjector/<.invoke@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:279524
bootstrap/vo@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:265983
bootstrap@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:266293
angularInit@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:265438
@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:40:474191
resolve/</Mr@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:3:49545
resolve/</qr<@https://treeherder.mozilla.org/vendor.779f1c423216e5768481.bundle.js:3:49964
  vendor.779f1c423216e5768481.bundle.js:40:336019

Comment 1

[Alice0775 White]

The problem seems to be beginning today(JST 2018-May-01).
It works as expected on Firefox59.0.3

Comment 2

[Alice0775 White]

Fixed range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5d5f51eff4380778908063d6fcd9bb9d73df1b11&tochange=88471e975cc350b28cc8ea651dbe7033db697ccd

So, treeheader seems to have been modified to use the bug 1317400 "Function.prototype.toString revision" feature which is not implemented on esr52.

Comment 4

[Ed Morley [:emorley]]

Summary of what I've found so far:
* that error seems to occur when AngularJS's class detection feature fails, which can happen when browsers deviate from the spec (see https://github.com/angular/angular.js/issues/14240#issuecomment-197418167)
* bug 1317400 fixed this spec deviation in Firefox 54, but it was never backported to ESR 52
* today's prod push was https://git.io/vpRZL 

So this is a browser bug triggered by recent Treeherder changes, and would need bug 1317400 backporting to ESR 52.

There are two more ESR 52 releases before it's EOL (52.8 on 2018-05-09, 52.9 on 2018-06-26), however the patchset in bug 1317400 comment 48 is pretty substantial, so I'm not sure they'd accept a backport. 

In addition, 2018-05-09 is when ESR 60 is being released anyway, at which point this is all moot.

It's worth noting as well that we intentionally don't target ESR in our Babel target browsers list (not that it would have helped with this issue), so from our side unless there was a quick workaround that someone else contributed, it's pretty much a wontfix.

Comment 5

[Tooru Fujisawa [:arai]]

I cannot locate the specific change that would cause this, in injector.js.
so I think it's the change in the caller's side. maybe the function passed there is changed to class ctor?
or maybe the code path that touches the function with class ctor is added recently?

maybe something similar to https://github.com/infiniteautomation/ma-dashboards/commit/513753ccb2caf0bc58e4ba6ae4a0bdae456f74d7 could help?
to skip the following branch.
https://github.com/angular/angular.js/blob/a7e5e83240f9539ffa0d4b429d30440c11775e76/src/auto/injector.js#L923-L926

Comment 6

[Tooru Fujisawa [:arai]]

Here's the prettified code of the class that is called there:
(the class in `controller` array)

the code is inside https://treeherder.mozilla.org/index.1aba9294b3da2bc98f7b.bundle.js , but I'm not sure where it's from :/
(at least it seems not to be in treeherder github)

> function(u, g, h) {
>     "use strict";
>     Object.defineProperty(g, "__esModule", {
>         value: !0
>     });
>     const E = h(1162),
>         S = h(1263),
>         T = h(1),
>         N = h(49);
>     g.react2angular = function(P, C = null, F = []) {
>         const A = C || P.propTypes && Object.keys(P.propTypes) || [];
>         return {
>             bindings: E(A.map((M) => [M, "<"])),
>             controller: ["$element", ...F, class extends S.default {
>                 constructor(M, ...R) {
>                     super(), this.$element = M, this.injectedProps = {}, F.forEach((I, L) => {
>                         this.injectedProps[I] = R[L]
>                     })
>                 }
>                 render() {
>                     N.render(T.createElement(P, Object.assign({}, this.injectedProps, this.props)), this.$element[0])
>                 }
>                 componentWillUnmount() {
>                     N.unmountComponentAtNode(this.$element[0])
>                 }
>             }]
>         }
>     }
> }

it doesn't have `$$ngIsClass` property in the definition, and it's set to false in injector.js (since the string representation doesn't start with "class" or "constructor").
Maybe adding $$ngIsClass property to the definition will help, if the code is controllable from our side.

Comment 7

[Tooru Fujisawa [:arai]]

about bug 1317400 (and maybe bug 1216630 too), it's a bit large patch series for uplift,
and the change was to support the spec update (so, iiuc, the previous behavior doesn't violate the spec at that point),
I'm not sure uplifting the change to ESR52 is the right thing to do here.

Comment 8

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #6)
> the code is inside
> https://treeherder.mozilla.org/index.1aba9294b3da2bc98f7b.bundle.js , but
> I'm not sure where it's from :/
> (at least it seems not to be in treeherder github)

possibly https://github.com/coatue-oss/react2angular/blob/master/index.tsx

so, the following changes are suspicious:
https://github.com/mozilla/treeherder/commit/4f2ee0c88b89b5fa9dcf0059308c4316d1df6e7c#diff-b9cfc7f2cdf78a7f4b91a753d10865a2
https://github.com/mozilla/treeherder/commit/7663ae2b671418262b5fb0f8847c9ec3f517f0c9#diff-b9cfc7f2cdf78a7f4b91a753d10865a2

If we can have local-modified version of react2angular which defines $$ngIsClass getter that returns true, it could be fixed.

Comment 9

[Ed Morley [:emorley]]

Yeah the switch to react2angular looks like what has made this start now. 

Though it's not really something we'd want to revert (or add hacks for in our repo) for 8 days of ESR 52 support before ESR 60 is released, given that ESR (along with mobile) is best-effort only. 

However if someone wants to submit a PR against react2angular to add the `$$ngIsClass` property, we'll gladly update as soon as a new version is released:
https://github.com/coatue-oss/react2angular/blob/master/index.tsx

Note that file is transpiled (since it's typescript), the actual content in the NPM package can be seen here:
https://unpkg.com/react2angular@3.2.1/index.es2015.js

To prototype a fix, I would:
1. Install node 8/9 (10 might work too, though not tested) and yarn
2. `git clone https://github.com/mozilla/treeherder`
3. `cd treeherder && yarn install && yarn start`
4. Edit `./node_modules/react2angular/index.es2015.js`
5. Wait for webpack to finish the recompile, then try to repro in ESR 52
6. Apply those changes upstream, adjusting for the typescript differences

(Or else `yarn link` react2angular and go for the full development/build workflow of it: https://github.com/coatue-oss/react2angular/blob/d0b8c336b2a52e05839636b7651dad959be76e7a/package.json#L9)

Comment 10

[Ed Morley [:emorley]]

To give some extra context on react2angular - we're mid rewriting all of our legacy AngularJS 1.6 components in React - and that library helps us do this piece by piece, before we then remove react2angular entirely at the end of the process. We were previously using ngReact to do the same, but that had other bugs that affected all browsers (eg bug 1451492).

Comment 11

[Tooru Fujisawa [:arai]]

https://github.com/coatue-oss/react2angular/pull/80

Comment 12

[Ed Morley [:emorley]]

Many thanks! (I know of at least one other Mozilla project that's about to start using angular2react, so working around this there is ideal)

Comment 14

[Botond Ballo [:botond]]

(In reply to Ed Morley [:emorley] from comment #4)
> In addition, 2018-05-09 is when ESR 60 is being released anyway, at which
> point this is all moot.

Technically, it's not moot until ESR52's end-of-life in August, since e.g. Linux distros could take up to that point to roll out an upgrade to ESR60.

Comment 15

[Ed Morley [:emorley]]

(In reply to Botond Ballo [:botond] from comment #14)
> Technically, it's not moot until ESR52's end-of-life in August, since e.g.
> Linux distros could take up to that point to roll out an upgrade to ESR60.

Agreed it's officially supported until August (we're sadly all too aware, since it's holding up bug 1443251).

However:
* The official Ubuntu firefox packages don't use ESR, so are already on v59 (even as far back as Trusty LTS).
* If there are distros whose official packages are ESR-only, then Firefox developers really shouldn't be using them for their _primary_ browser, since:
  - there have been multiple cases where ESR has missed security updates that were shipped on release (eg the recent incident)
  - Firefox developers should be dogfooding at the least Firefox stable, and ideally Beta or Nightly.
* Treeherder makes no guarantee of support for even the latest ESR - so support for previous ESR should be seen as non-existent.

Anyway I'm really grateful to Tooru for opening the PR against react2angular - and if/when that's merged and released, the Renovate bot (https://renovateapp.com) will open a PR against Treeherder to pick up the update - which I'll merge/deploy straight away.

Comment 16

[Ed Morley [:emorley]]

The react2angular fix has been merged and released in react2angular v4.0.1, which Treeherder was updated to in:
https://github.com/mozilla/treeherder/pull/3525

I've just deployed that to production, so ESR 52 should work there now.