1336556 - Use Neutrino/WebPack instead of Grunt to do our build and minification

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P2)

Description

[Cameron Dawson [:camd]]

WebPack is supposed to be easier to use with better options.  It will also make it possible to use JSX with ReactJS components.

Comment 1

[Ed Morley [:emorley]]

Hi Casey! I can't remember from the treeherder meeting a week or so ago if you said you'd started working on this, or whether it was something for the future? If bug 1242905 is taking priority that's absolutely fine - I'm just hoping to plan timeline of this since we'll need it for bug 1337488 :-)

Comment 2

[Casey Williams]

So far I've been able to completely replace grunt for the purposes of running a dev server and building the production version of the code (you can see the config in this commit, on my fork: https://github.com/caseywilliams/treeherder/commit/f606d3b802c03bd595d3c617e1308f9f00e611e5). Happily, it allows us to use jsx for the react components and the production bundle is about 5.2 MB smaller than before. But getting the test configuration right is much trickier, since there don't seem to be many examples of how to make webpack work in concert with karma, babel, jasmine, and angular 1.5. I'll be talking with Eli early next week about how to get this ready to review - I'm hopeful I can make a PR by next Wednesday.

Comment 3

[GitHub Autolander Bot]

Attachment: [treeherder] caseywilliams:neutrino > mozilla:master

Comment 4

[Casey Williams]

(In reply to Autolander from comment #3)
> Created attachment 8836734 [details] [review]
> [treeherder] caseywilliams:neutrino > mozilla:master

I believe this commit successfully replaces grunt with webpack/neutrino (except for e2e ui tests -- see below -- and a caveat about running vagrant with unminified code). Some notes:
 
The dev server:

- `npm start` will start a webpack-dev-server via neutrino, communicating with the prod api. This is the equivalent of running ./web-server.js under grunt.
- `npm run start:stage` will do the same, but communicate with staging.
- To use a different service domain, you can set the SERVICE_DOMAIN environment variable and run `npm start`.

Building/minification:

- `npm run build` will build the minified version for dist/.
- I converted several self-closing tags to paired tags in some template HTML - these were cases where the minifier was unhappy for whatever reason.
- There were also a few places where strict dependency injection wasn't being used with angular modules, which broke minification as well. I fixed them and added the `ng-strict` attribute to the various `ng-app` elements to enforce this.

Vagrant caveat:

As always, the Vagrant server will read from dist/ when SERVE_MINIFIED_UI is set, but viewing the unminified version via vagrant does not work, since webpack must build the code for it to run correctly -- that is, only the minified version can be viewed via vagrant right now. I think the easiest way to get around this may be to just point the webpack-dev-server at the api url for vagrant, instead of having webpack build unminified code specifically for vagrant - does that sound acceptable? 

JSX:

I converted the revision list react component to jsx as an example. I'll probably do this for the react list component in the admin app as well.

Tests:

- `npm test` will run the tests via neutrino. This was tricky to set up -- at this point it seems that due to a bug in karma-webpack (https://github.com/webpack-contrib/karma-webpack/issues/40), it's not possible to just list tests in the karma config - I had to do this in an entry file in the unit test directory to them run successfully.
- `npm test:watch` will watch tests and run them on changes, but because of the aforementioned bug, it runs *all* the tests each time a file changes, which is quite slow. It may be possible to fix this once the bug is fixed and we can list files in the karma config instead of an entry file (or maybe there's just something I'm missing here).
- This setup will not run the e2e tests for the frontend right now. There's a problem in that the ng-scenario tests require an html runner file to function, but neutrino seems to strip out any plugins (such as an html-webpack-plugin which could generate such a file) from the karma config before it runs the tests, so I'm not sure how to make it work. On the other hand, angular now suggests using protractor instead of ng-scenario for e2e tests (see https://docs.angularjs.org/guide/e2e-testing). I'm thinking that it may be easier to get e2e tests running if I just convert it all to protractor instead of fiddling with ng-scenario (there are only a handful of them) - any thoughts?

Comment 5

[William Lachance (:wlach)]

(In reply to Casey Williams from comment #4)
> (In reply to Autolander from comment #3)
> > Created attachment 8836734 [details] [review]
> > [treeherder] caseywilliams:neutrino > mozilla:master
> 
> I believe this commit successfully replaces grunt with webpack/neutrino
> (except for e2e ui tests -- see below -- and a caveat about running vagrant
> with unminified code). Some notes:
>  
> The dev server:
> 
> - `npm start` will start a webpack-dev-server via neutrino, communicating
> with the prod api. This is the equivalent of running ./web-server.js under
> grunt.
> - `npm run start:stage` will do the same, but communicate with staging.
> - To use a different service domain, you can set the SERVICE_DOMAIN
> environment variable and run `npm start`.

Could you please update the docs with information on this change? 

http://treeherder.readthedocs.io/ui/installation.html

(there may be other parts of the docs which also need updating, but this stood out)

Comment 6

[Casey Williams]

(In reply to William Lachance (:wlach) (use needinfo!) from comment #5)
> (In reply to Casey Williams from comment #4)
> > (In reply to Autolander from comment #3)
> > > Created attachment 8836734 [details] [review]
> > > [treeherder] caseywilliams:neutrino > mozilla:master
> > 
> > I believe this commit successfully replaces grunt with webpack/neutrino
> > (except for e2e ui tests -- see below -- and a caveat about running vagrant
> > with unminified code). Some notes:
> >  
> > The dev server:
> > 
> > - `npm start` will start a webpack-dev-server via neutrino, communicating
> > with the prod api. This is the equivalent of running ./web-server.js under
> > grunt.
> > - `npm run start:stage` will do the same, but communicate with staging.
> > - To use a different service domain, you can set the SERVICE_DOMAIN
> > environment variable and run `npm start`.
> 
> Could you please update the docs with information on this change? 
> 
> http://treeherder.readthedocs.io/ui/installation.html
> 
> (there may be other parts of the docs which also need updating, but this
> stood out)

I've updated the PR with documentation on how to run all of the new npm tasks. At this point, it's possible to run the webpack dev server locally to serve the unminified files and have it communicate with the vagrant backend for data by setting the service domain to localhost:8000. But since the webpack dev server runs on a different port from the vagrant server, it still isn't possible to log in unless you run the minified version of the code, which is statically served out of dist/ by vagrant after webpack builds it. 

I will continue to think about how to allow logins with the unminified webpack ui - a solution may be to add a new npm task which watches ui files and builds an unminified version of the code in dist/ each time they change, allowing the vagrant server to serve the unminified files -- I'll see how that works.

Comment 7

[William Lachance (:wlach)]

(In reply to Casey Williams from comment #6)
> I've updated the PR with documentation on how to run all of the new npm
> tasks. At this point, it's possible to run the webpack dev server locally to
> serve the unminified files and have it communicate with the vagrant backend
> for data by setting the service domain to localhost:8000. But since the
> webpack dev server runs on a different port from the vagrant server, it
> still isn't possible to log in unless you run the minified version of the
> code, which is statically served out of dist/ by vagrant after webpack
> builds it. 

Docs changes look good, thanks! I left one comment in the PR.

Comment 8

[Casey Williams]

A new update:

At Eli’s suggestion, I’ve parted the neutrino configs into more modular chunks, which are now stored in neutrino-presets/. I’ve also updated the docs with information on how to run the new npm tasks for this build system. Here’s a short summary:

npm start
	runs the development server (equivalent to ./web-server.js previously)

npm run build
	runs the production build (equivalent to grunt build previously)

npm run start:local
	starts a webpack build that watches ui/ and re-builds an unminified version in dist/ on file changes. This is used to serve the unminified code from the vagrant site (run `./manage.py runserver` inside of vagrant and then run this task). It’s a little slower than `npm start`’s webpack-dev-server version, but it allows logging into the frontend of the vagrant site because it’s served from the same origin.

npm test
	runs unit tests once

npm run test:watch
	runs unit tests once and then watches ui/, running tests again on changes.

At this point I believe the only things left to address are:

1. The question on e2e tests from my previous comment
2. The selenium tests are failing, but I’m not sure why (e.g. https://travis-ci.org/mozilla/treeherder/jobs/203016683). For example, Travis reports that selenium can’t find the “Add more test data” button under the Perfherder Graphs tab, but it seems to work in all of my local tests.
2. I’m not entirely sure that my edits to `bin/post_compile` and `treeherder/config/whitenoise-custom.py` are appropriate - it would be great to get a second opinion on those.

Comment 9

[William Lachance (:wlach)]

(In reply to Casey Williams from comment #8)
> 2. The selenium tests are failing, but I’m not sure why (e.g.
> https://travis-ci.org/mozilla/treeherder/jobs/203016683). For example,
> Travis reports that selenium can’t find the “Add more test data” button
> under the Perfherder Graphs tab, but it seems to work in all of my local
> tests.

The closest local analog to running the selenium tests is running a development server via './manage.py runserver'. Does that work?

Comment 10

[Casey Williams]

> The closest local analog to running the selenium tests is running a
> development server via './manage.py runserver'. Does that work?

Yep, the minified and unminified UI both seem to work fine when served via ./manage.py runserver.

Comment 11

[William Lachance (:wlach)]

(In reply to Casey Williams from comment #10)
> > The closest local analog to running the selenium tests is running a
> > development server via './manage.py runserver'. Does that work?
> 
> Yep, the minified and unminified UI both seem to work fine when served via
> ./manage.py runserver.

So I tried merging the branch in and running the dev server (./manage.py runserver). It seems like when I load the test data chooser, it doesn't have the `performance-test-chooser` id that I'd expect it to from the file (and which selenium is looking for):

https://github.com/mozilla/treeherder/blob/master/ui/partials/perf/testdatachooser.html#L9

Based on the network console, it looks like the minified version of the assets were being served. Hope that gives a hint on where to look...

Comment 12

[Casey Williams]

I've just pushed a revision of this commit that uses the newly released neutrino v4 instead of v3. The npm tasks for running/testing/building are the same as before. I'm afraid there are still two issues I'm not sure how to solve:

1. I managed to run the selenium tests locally, and fixed the timeout error I was getting by doubling the WebDriverWait time in test_basics.py. This fix did not work on Travis, though - I think I'm out of ideas as to how to really fix it now.

2. All the npm tasks work for me locally, even after removing and reinstalling all node modules, but in travis the ui test run fails, claiming that one of the babel presets used in Neutrino (babel-preset-env) can't find lodash/intersection. Treeherder itself uses an older version of lodash than babel-preset-env, but that shouldn't be a problem if all of babel-preset-env's dependencies were installed. I'm not sure what could cause this to happen.

Finally, there are two smallish cosmetic problems that will likely be solved by another update to Neutrino in the coming weeks/months. These don't actually affect the run/build process, but they should probably be noted:

- There is a deprecation warning that appears in npm task output - this seems to be a problem in one of Neutrino's loader dependencies, not Neutrino itself.
- Neutrino does not produce any output when running a webpack watch (i.e. watching and building in dist, not the default dev server) via `npm run start:local`. There's already a github issue for this, and it will likely be fixed soon.

Comment 13

[:Eli Perelman]

> claiming that one of the babel presets used in Neutrino (babel-preset-env) can't find lodash/intersection

This was a bug in babel-preset-env that was just regressed and fixed. I have pushed out an updated version of the preset to fix.

Comment 14

[:Eli Perelman]

> There is a deprecation warning that appears in npm task output

This is now gone as well.

Comment 15

[William Lachance (:wlach)]

(In reply to Casey Williams from comment #12)
> 1. I managed to run the selenium tests locally, and fixed the timeout error
> I was getting by doubling the WebDriverWait time in test_basics.py. This fix
> did not work on Travis, though - I think I'm out of ideas as to how to
> really fix it now.

So looking at the code, I guess the question I have is how are the static assets now being served for a development server (the ./manage.py runserver case). It appears that we no longer have the SERVE_MINIFIED_UI option.

My *guess* is that either it's taking too long to compile and serve the assets or they aren't being compiled and served at all in the vagrant case.

Comment 16

[Casey Williams]

(In reply to William Lachance (:wlach) (use needinfo!) from comment #15)
> So looking at the code, I guess the question I have is how are the static
> assets now being served for a development server (the ./manage.py runserver
> case). It appears that we no longer have the SERVE_MINIFIED_UI option.
> 
> My *guess* is that either it's taking too long to compile and serve the
> assets or they aren't being compiled and served at all in the vagrant case.

Ah! I was assuming it was trying to test the built/minified assets - This is almost certainly the problem, then. Webpack typically serves unminified assets through its own little development server, webpack-dev-server, which can be configured to talk to a backend of your choice. I used this development server for:

- `npm start` (webpack-dev-server communicating with production TH instance)
- `npm run start:stage` (webpack-dev-server communicating with staging TH instance)

These seem to run faster than a watcher that builds an unminified version on code changes. But running a development server on a separate port like this presents a problem for Taskcluster logins, which are rejected when coming from a different origin. To get around this and provide a way to log into the frontend without having to build the minified version, I also added a watcher (`npm run start:local`) which watches for changes and outputs an unminified version to dist/. This means that even when vagrant is serving unminified assets, it's still looking in dist/ the way it does when it's serving minified assets - that was the reasoning behind getting rid of SERVE_MINIFIED_UI.

Would it help if I added a task to do a single build of unminified assets in dist/ that could run before the selenium tests?

Comment 17

[William Lachance (:wlach)]

(In reply to Casey Williams from comment #16)
> (In reply to William Lachance (:wlach) (use needinfo!) from comment #15)

> Would it help if I added a task to do a single build of unminified assets in
> dist/ that could run before the selenium tests?

Hmm, good question, I'm not entirely sure what the best solution is here. The ideal would be for there to be no big "gotchas" in the execution of the selenium tests (having to run some kind of command before executing the tests is a bit of a gotcha). So I think the ideal would be for the unit test itself to run the command and perform the setup via a fixture (probably doing something clever so we don't have to perform that operation for every selenium test).

However, in the interests of getting something landed sooner, I think it would be acceptable to add that task and get travis to execute it. :) We can file a bug as a followup to do something better later.

Comment 18

[Casey Williams]

Okay, I've updated the Travis config to install the right version of node via nvm and build the frontend assets before it starts the selenium tests, and now they pass. I've also updated neutrino and its various presets to their latest versions, and the babel-preset-env bug in running ui tests is gone. Thanks all!

Comment 19

[Ed Morley [:emorley]]

I'm having a closer look through the PR now - the overall shape of this is great - can't wait for this to land!

In the meantime would you mind splitting out some of the cleanup changes to a separate commit or PR, just to simplify review? (It will also reduce risk of landing and chance of later conflicts should we need to revert) For example the whitespace/markup fixes, end to end test removal, and anything else that can stand on its own.

Comment 20

[GitHub Autolander Bot]

Attachment: [treeherder] caseywilliams:neutrino-prep > mozilla:master

Comment 21

[Casey Williams]

(In reply to Autolander from comment #20)
> Created attachment 8842462 [details] [review]
> [treeherder] caseywilliams:neutrino-prep > mozilla:master

I've broken my initial PR down into two: this one makes some preparatory changes to markup and javascript and removes some defunct tests and scripts. It leaves grunt intact, so it can be safely applied first. The second, original PR will apply the actual neutrino/webpack conversion.

Comment 22

[Cameron Dawson [:camd]]

Comment on attachment 8836734 [details] [review]
[treeherder] caseywilliams:neutrino > mozilla:master

Fantastic work!  I am only sort of understanding Neutrino, so very glad Eli could review this as well.  :)

Comment 23

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/c4427609a34e141685b57339b5730d4bb181c5e7
Bug 1336556 - Remove unused UI tests

Remove old js test stubs and e2e tests superseded by selenium

https://github.com/mozilla/treeherder/commit/b2e9e7545712647ec28ccfb9f4c7f7c22a209fdb
Bug 1336556 - Make UI test naming more consistent

https://github.com/mozilla/treeherder/commit/de1137aeb8a61223569b9bc1f1d755451308788c
Bug 1336556 - Apply strict DI in angular controllers & apps

This will prevent some minification errors after the migration from grunt
to webpack.

https://github.com/mozilla/treeherder/commit/a448fe0db6da7834bd71772b89bd9b8f3667f73f
Bug 1336556 - Add karma coverage directory to .gitignore

https://github.com/mozilla/treeherder/commit/49b60d3c54a40a6cda5f0efd8f50f9b58a241485
Bug 1336556 - Use `angular.mock.module` over `module`

Access angular.mock.module in UI tests using its full name, so that
`module` doesn't conflict with node's `module` after the webpack
migration.

https://github.com/mozilla/treeherder/commit/009d9f98b02c93336d5c7fed608985c3b5e6cb8b
Bug 1336556 - Pair up a few self-closed tags

These seem to cause visual bugs in some minified outputs

https://github.com/mozilla/treeherder/commit/f146f517c8e985ea8b68bd083df9e7ab807bfe58
Bug 1336556 - Use unminified jquery.flot.selection.js

Babel minification does better when starting with unminified code

https://github.com/mozilla/treeherder/commit/a589a3dece0a453b2844922212fe21d866f72496
Bug 1336556 - Include navbar-left span wrap in perf & admin html

This has no visual effect right now, but when minifying with webpack,
the dropdowns in the perf and admin headers appear incorrectly offset if
they're not wrapped in this class.

https://github.com/mozilla/treeherder/commit/9aae5c67d6a13f36a1bf0e1bacbe9177827d7522
Bug 1336556 - Check port for cross-origin in auth.js

In addition to hostname

Comment 24

[Ed Morley [:emorley]]

Eli, I don't suppose you could help with the eslint errors Casey is seeing? :-)
https://github.com/mozilla/treeherder/pull/2159#issuecomment-286899905

Comment 25

[:Eli Perelman]

Addressed on GitHub.

Comment 26

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/11ca87fb3444f87981a3c751bd195758183222b4
Bug 1336556 - Update eslint to v3.17.1

Updates eslint in advance of grunt -> webpack switch; Fixes some
resulting newly detected indentation issues.

https://github.com/mozilla/treeherder/commit/afa7d63d1d94462d04028703d7e734b47c333fd2
Bug 1336556 - Replace grunt build system with neutrino/webpack

https://github.com/mozilla/treeherder/commit/2f728b6ea4ce9d9b1da778b9990a1ac6f0212068
Bug 1336556 - Remove unused vendor files in UI

Most libraries are provided by npm now

https://github.com/mozilla/treeherder/commit/c19bfb2da9462353ee6d93f2cfa2696df9146797
Bug 1336556 - Cache angular partials with webpack

Load the contents of the various angular partials into the template
cache at build time so that they don't need to be built separately.

https://github.com/mozilla/treeherder/commit/5a1c8c6af3721fc724c452a4d699f6422c00a9c3
Bug 1336556 - Add a standalone eslint task

Installs eslint explicitly to avoid warning messages on initial
node_modules install.  Adds a standalone eslint task via `yarn run
lint`.

Comment 27

[Ed Morley [:emorley]]

Auto-deployed to stage, will be on prod soon.

Thank you for all your work on this Casey!

Once Neutrino v5 is released I'll file a bug for switching to it, since it will allow for some simplification of some of the configs.

Comment 29

[GitHub Autolander Bot]

Attachment: [treeherder] KWierso:fixafterlint > mozilla:master

Comment 30

[Ed Morley [:emorley]]

Comment on attachment 8854057 [details] [review]
[treeherder] KWierso:fixafterlint > mozilla:master

This line was one of the merge conflicts, guessing was broken during rebase. Thank you for fixing :-)

Comment 31

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/b2c1d690c1cf1f18d2bb181bdc5db3bee0495a38
Bug 1336556 - followup to fix active-filters-bar (#2311) r=emorley