Closed
Bug 1458017
Opened 3 years ago
Closed 3 years ago
Treeherder's UI does not work on ESR 52 due to react2angular ("TypeError: class constructors must be invoked with |new|")
Categories
(Tree Management :: Treeherder: Frontend, defect, P5)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: alice0775, Unassigned)
References
()
Details
(Keywords: regression)
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
![]() |
Reporter | |
Comment 1•3 years ago
|
||
The problem seems to be beginning today(JST 2018-May-01). It works as expected on Firefox59.0.3
Keywords: regression
![]() |
Reporter | |
Comment 2•3 years ago
|
||
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.
Blocks: 1317400
Flags: needinfo?(arai.unmht)
![]() |
Reporter | |
Updated•3 years ago
|
Component: Treeherder → Treeherder: Frontend
Comment 4•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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.
Flags: needinfo?(arai.unmht)
Comment 7•3 years ago
|
||
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•3 years ago
|
||
(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•3 years ago
|
||
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•3 years ago
|
||
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 12•3 years ago
|
||
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•3 years ago
|
||
(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•3 years ago
|
||
(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.
Blocks: 1408791
Priority: -- → P5
Summary: treeherder.mozilla.org does not work on ESR52.7.4 → Treeherder's UI does not work on ESR 52 due to react2angular ("TypeError: class constructors must be invoked with |new|")
Comment 16•3 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•