1408784 - Upgrade to Angular UI-Router 1.0

Status: RESOLVED WONTFIX

(Tree Management :: Perfherder, enhancement, P3)

Description

[Ed Morley [:emorley]]

We're currently using 0.4.x of:
https://www.npmjs.com/package/angular-ui-router

The 1.0+ version has been released under a new NPM package name, which is why it hasn't appeared on updated package listed before.

See:
https://ui-router.github.io/guide/ng1/migrate-to-1_0
https://www.npmjs.com/package/@uirouter/angularjs

Comment 1

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3072

Comment 2

[Ed Morley [:emorley]]

To help with testing this PR (and a bunch of others, eg React 16, Angular 1.6) all are deployed together to:
https://treeherder-prototype.herokuapp.com/

Comment 3

[William Lachance (:wlach)]

Comment on attachment 8938976 [details] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3072

In principle I am fine with this change but I get console errors when I try to (re)load this url:

https://treeherder-prototype.herokuapp.com/perf.html#/graphs?series=mozilla-inbound,1315470,1,1
​
fileName: "https://treeherder-prototype.herokuapp.com/perf.b79bb7d2f65138a9556e.bundle.js"
​
lineNumber: 45
​
message: "\"series\" is read-only"
​
stack: "[606]/</</<@https://treeherder-prototype.herokuapp.com/perf.b79bb7d2f65138a9556e.bundle.js:45:65935\nprocessQueue@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:169193\nscheduleProcessQueue/<@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:169859\n$digest@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:177369\n$apply@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:179480\nscheduleApplyAsync/Do<@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:174608\ncompleteOutstandingRequest@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:107375\nBrowser/Ro.defer/Zo<@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:109293\nsetTimeout handler*Browser/Ro.defer@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:109265\nscheduleApplyAsync@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:174588\n$applyAsync@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:179607\ndone@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:146015\ncompleteRequest@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:149662\ncreateHttpBackend/</Go.onload@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:150180\nEventHandlerNonNull*createHttpBackend/<@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:149994\nsendReq@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:147380\n$http/Go<@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:145426\nprocessQueue@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:169193\nscheduleProcessQueue/<@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:169859\n$digest@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:177369\n$apply@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:179480\nRo/<@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:90945\ninvoke@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:104402\nRo@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:90861\nbootstrap@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:91171\nangularInit@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:90316\n@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:298083\nHr@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:60417\nresolve/</zr<@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:60847\nsetTimeout handler*resolve/<@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:61080\nMr@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:58493\nfireWith@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:59359\nfire@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:59394\nMr@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:58493\nfireWith@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:59359\nready@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:62697\ncompleted@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:14982\nEventListener.handleEvent*@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:62833\n@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:13454\n@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:13:13353\n__webpack_require__@https://treeherder-prototype.herokuapp.com/manifest.0eea2586d68f450f101b.bundle.js:1:114\n@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:51:298499\n__webpack_require__@https://treeherder-prototype.herokuapp.com/manifest.0eea2586d68f450f101b.bundle.js:1:114\n@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:18:9518\n__webpack_require__@https://treeherder-prototype.herokuapp.com/manifest.0eea2586d68f450f101b.bundle.js:1:114\n@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:24:180997\n@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:24:180975\n__webpack_require__@https://treeherder-prototype.herokuapp.com/manifest.0eea2586d68f450f101b.bundle.js:1:114\n@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:65:222682\n__webpack_require__@https://treeherder-prototype.herokuapp.com/manifest.0eea2586d68f450f101b.bundle.js:1:114\nwindow.webpackJsonp@https://treeherder-prototype.herokuapp.com/manifest.0eea2586d68f450f101b.bundle.js:1:454\n@https://treeherder-prototype.herokuapp.com/vendor.e74f7a32f34ee9a18ac5.bundle.js:1:1\n"
​
__proto__: Object { stack: "", … }
 Possibly unhandled rejection: {}
angular.js:14791

Comment 4

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) (use needinfo!) from comment #3)
> Comment on attachment 8938976 [details] [review]
> Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3072
> 
> In principle I am fine with this change but I get console errors when I try
> to (re)load this url:
> 
> https://treeherder-prototype.herokuapp.com/perf.html#/graphs?series=mozilla-
> inbound,1315470,1,1
> ​...

On second think, I guess I actually don't know if that's due to the router upgrade or not. But I guess the error will need to be fixed one or the other (FWIW this operation works fine on treeherder stage).

Comment 5

[Ed Morley [:emorley]]

Ah trying locally with `yarn start` (which doesn't do all the minification of the prod build), it shows the exception as originating from here:

    $stateParams.series = [$stateParams.series];

(https://github.com/mozilla/treeherder/blob/19a0cfdde8c2a005a42ce91f32c77ee41e19b195/ui/js/controllers/perf/graphs.js#L755)

Looking at the migration docs, $stateParams was deprecated:
https://ui-router.github.io/guide/ng1/migrate-to-1_0#stateparams-deprecation

...though there was no mention of its behaviour changing.

It seems that perhaps it's now read-only - and given that `$stateParams.series` is only used in a few places, that a local variable could be used instead?

Though it would also seem worth filing a bug to get the migration docs fixed to mention the change :-/

Possibly related:
https://github.com/angular-ui/ui-router/issues/3506

Comment 6

[William Lachance (:wlach)]

(In reply to Ed Morley [:emorley] from comment #5)
> ...
> It seems that perhaps it's now read-only - and given that
> `$stateParams.series` is only used in a few places, that a local variable
> could be used instead?

$stateParams is used extensively to serialize/deserialize router state into the controller, so it's not that straightforward I'm afraid. You'll presumably have to migrate to the $transition$ object as they suggest.

Personally I would recommend just leaving this as-is unless there's some other compelling reason to upgrade.

Comment 7

[Ed Morley [:emorley]]

(In reply to William Lachance (:wlach) (use needinfo!) from comment #6)
> Personally I would recommend just leaving this as-is unless there's some
> other compelling reason to upgrade.

The old version has an unfortunate broken tilde range dependency on angular 1.x, whereas the new one correctly has a peerDependency. Once we upgrade to newer Neutrino this becomes an issue, because a bug was fixed there that means module resolution now happens correctly. If we were still using Angular 1.5 at the time of updating Neutrino, we'd end up bundling two copies of Angular (1.5 + 1.6) in the frontend, which in addition to increased payload size, results in a "angular initialised twice" error.

That said, if we update to Angular 1.6 before the Neutrino upgrade, and then always make sure we stay on the latest Angular 1.x major version, then with yarn hoisting we can avoid the issue for now.

Ideally at some point in the future Perfherder will be written using React and its routing solution, and all this will become moot :-)

Comment 8

[William Lachance (:wlach)]

(In reply to Ed Morley [:emorley] from comment #7)
> (In reply to William Lachance (:wlach) (use needinfo!) from comment #6)
> > Personally I would recommend just leaving this as-is unless there's some
> > other compelling reason to upgrade.
> 
> The old version has an unfortunate broken tilde range dependency on angular
> 1.x, whereas the new one correctly has a peerDependency. Once we upgrade to
> newer Neutrino this becomes an issue, because a bug was fixed there that
> means module resolution now happens correctly. If we were still using
> Angular 1.5 at the time of updating Neutrino, we'd end up bundling two
> copies of Angular (1.5 + 1.6) in the frontend, which in addition to
> increased payload size, results in a "angular initialised twice" error.
> 
> That said, if we update to Angular 1.6 before the Neutrino upgrade, and then
> always make sure we stay on the latest Angular 1.x major version, then with
> yarn hoisting we can avoid the issue for now.

Ok. :) In that case it may be worthwhile fixing this. Hopefully it's not too ridiculous (may just be a matter of changing some variable names?).

> Ideally at some point in the future Perfherder will be written using React
> and its routing solution, and all this will become moot :-)

Indeed!

Comment 9

[Ed Morley [:emorley]]

Let's wontfix this in favour of switching to React and some react based routing solution.

Comment 10

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/ebc28494ffc9683bbcfdf4875a8436cb3a0db5ae
Switch to the new package name for angular-ui-router (#4074)

The `angular-ui-router` package has been renamed, but otherwise the
contents are identical:
https://registry.npmjs.org/angular-ui-router/-/angular-ui-router-0.4.3.tgz
https://registry.npmjs.org/@uirouter/angularjs/-/angularjs-0.4.3.tgz

Fixes:

```
warning angular-ui-router@0.4.3:
This npm package 'angular-ui-router' has been renamed to
'@uirouter/angularjs'. Please update your package.json. See
https://ui-router.github.io/blog/uirouter-scoped-packages/
```

I've also added the package to the Renovate ignore list, since
otherwise Renovate will open a new PR due to the name change, and
we're intentionally not updating it (see #4052 and bug 1408784).