Simplify and improve UXPL development/build process #367

bjacobel · 2016-09-19T18:26:06Z

Simplify and improve UXPL development/build process

Description

Sped up asset compilation dramatically. Before we were looking at a minimum of 35 seconds every time you re-ran pldoc-styles, in addition to the time to copy other assets. Now, Webpack can make a production bundle of all the assets (Javascript, fonts, CSS) in around 30 seconds from cold start - but, much better, there is now a development mode that loads up in 19 seconds and caches/recompiles modules based on the contents that are diffed. The new time to recompile a change of the pattern library sass and get it into a new webpack bundle is about 16 seconds, which we know to be the same speed as running node-sass directly on the Pattern Library root, so as we optimize that process (separate ticket to upgrade Susy) that time should drop as well.
Removed all content from and references to the demo. It was something Brian Talbot created that hadn't been used or modified since, and in my opinion was superseded by the pattern library documentation but continued to exist alongside (actually, inside) it.
Streamlined the asset pipeline, bridged gaps and removed dead paths. Issues like FEDX-216, where assets were rebuilt but never sent to the browser, and the issue where pldoc previews would not get any fonts (no ticket but longstanding annoyance) are now fixed. This involved removing a lot of Gulp tasks that didn't do anything, as far as I can tell. The basic Gulp entry point still builds Jekyll and starts Webpack, but some of the intermediate steps that I don't think anybody was calling directly are gone. If I broke your workflow, let me know.
Removed the step of the build process where we would lint your javascript before rebundling it because it was slowing us down. I think that's the job of an editor/IDE, not an asset pipeline.
Implemented webpack-dev-server to take advantage of hot module reload and Webpack module caching. Now when you save a file, nothing ever needs to touch the filesystem - WDS invokes Webpack, figures out what modules need to be rebuilt based on the dependency graph, triggers a hot module reload on the browser, serves the emitted assets out of memory and proxies all the non-Webpack things through to the Jekyll server.
As a result of the above change, the doc site now comes up at localhost:8080 rather than localhost:4000. Added some documentation to the server's startup messages about this.
Removed a ton of stuff from package.json - some we weren't using at all and some we didn't need anymore.
Moved all the Webpack config into webpack.config.js - previously it was generated dynamically in one of the gulp tasks. Now it's much more parseable to see what exact settings Webpack is going to get when you start it up, and you can run it from the command line as well.
The above enabled making it so that you don't need Gulp to run the development site - there's a script in npm start that does all the same stuff as $(npm bin)/gulp, even a little faster I think.
Make Travis lint and try to make a production build on every push and PR, using both node 4 (LTS, current support, uses NPM 2) and node 6 (stable, migration planned soon, uses NPM 3)
Removed _zappr.yml - I forgot to remove it when we turned that off.

Basically we're making Webpack handle as much of this asset processing/bundling stuff as possible, because it's great at it and we get speed boosts when we stay inside the ecosystem. Gulp's role is reduced because we don't need two asset managers and Webpack is better at it 😜

Known issues

In development, the doc site throws a 404 for pattern-library-doc.css. This is because there is no separate CSS file built in development (the CSS is completely bundled inside the Javascript, so it can be reloaded with HMR) but it would be too complex to have two different Jekyll templates - one that requests the CSS file for production, and one that doesn't for development. So in development we request the CSS file anyway, knowing it will fail and that the Webpacked CSS will be there inside the JS bundle to style the page anyway.
In development, there is a short flash of unstyled content (FOUC) when we navigate from page to page. This is because we have a hybrid application (server-side HTML, but client-side JS/CSS) and the HTML loads before the JS bundle (containing the CSS) is fully unpacked. The benefit to doing it this way is that we get Hot Module Reload in development, which is awesome and faster than BrowserSync. Living with the FOUC also allows us to have shorter asset build times - otherwise we have to use ExtractTextPlugin (which we do for the prod build) which is ~2x as slow at building Sass.
I haven't yet tested gulp doc-publish. I'll do it after I get thumbs here, because it will wipe out the existing doc site.

Sandbox

http://ux-test.edx.org/bjacobel/fix-css/

Testing Checklist

Manually test responsive behavior.
N/A Manually test right-to-left behavior. (We don't currently build an RTL version of the pattern library documentation, but I have an idea of how we can do it without slowing things down too much using this new webpack architecture).
Manually test a11y support.

Non-testing Checklist

Consider any documentation your change might need, and which users will be affected by this change.

Reviewers

If you've been tagged for review, please check your corresponding box once you've given the 👍.

mention-bot · 2016-09-19T18:26:07Z

@bjacobel, thanks for your PR! By analyzing the annotation information on this pull request, we identified @andy-armstrong and @alisan617 to be potential reviewers

andy-armstrong · 2016-09-19T20:22:20Z

gulp/tasks/webpack.js

+    wpConfigFactory = wpConfig.configFactory,
+    wpConfigDevServer = wpConfig.devServerConfig;
+
+/* Builds the pattern library doc into /pldoc/public.


Nit: I've never seen multi-line comments like this in our codebase. We either use // or do a JSDoc-style comment starting with /**:

http://edx.readthedocs.io/projects/edx-developer-guide/en/latest/style_guides/javascript_guidelines.html#documentation

Not a big deal if you prefer doing it like this, but just fyi.

I would probably make these JSDoc-like, with a single line summary as the first line. They aren't defining functions per se, but they are defining conceptual blocks.

andy-armstrong

Wow @bjacobel. This is an incredibly impressive cleanup. I was convinced that you must have broken preview, but it all looks good to me. Major 👍 from me.

roderickmorales · 2016-09-20T16:18:45Z

@bjacobel my only question is around the "demo" directory. Was that an attempt on Brian to have static samples of our patterns to match production? I think something like that will be valuable to have but maybe we can discuss how to set it up, so it makes sense to everyone.

👍

dsjen

Thanks for speeding this up! 🔨 🐺 A bit tangential is that we'll probably need a follow up task to see if we can get it down from 16s (w/ susy modifications?) and also it seems like the speed up has been in improvements through webpack, whereas Insights won't pick up that speed increase because it's using django-libsass--not sure what ecom is doing, but I suspect something similar.

dsjen · 2016-09-20T19:36:43Z

gulp/config.js

-        // example demo
-        demo_src: demoSrc + '/static/images',
-        demo_src_files: demoSrc + '/static/images/**/*'
+        pldoc_src_files: pldocSrc + '/static/images/**/*'


I know that you're just following the previous style here. Why not use camelCase?

This is just a +diff because I removed the comma at the end of the line, but yep - we should absolutely be using camelCase for these (or removing them if they're no longer used, which I think these fonts ones might not be). I'll see what I can do with cleaning this file up 👍

Yep, this file ended up being used in like two places. I think it's adding more complexity than it's solving, so it's gone now!

dsjen · 2016-09-20T19:37:45Z

gulp/tasks/build-production.js

@@ -1,14 +1,9 @@
 'use strict';

 var gulp = require('gulp'),
-    runSequence = require('run-sequence');
+    gulpUtil = require('gulp-util');


Is gulpUtil needed?

dsjen · 2016-09-20T19:38:07Z

gulp/tasks/default.js

@@ -1,12 +1,9 @@
 'use strict';

 var gulp = require('gulp'),
-    runSequence = require('run-sequence');
+    gulpUtil = require('gulp-util');


Same question on gulpUtil.

dsjen · 2016-09-20T19:43:24Z

gulp/tasks/preview.js

    var branch = gitUtils.currentBranch(),
        previewBaseUrl = '/' + branch + '/';
+
+


Total nitty nit: too many spaces.

dsjen · 2016-09-20T19:46:51Z

gulp/tasks/webpack.js

+    });
+});
+
+/* Builds the pattern library documentation using webpack and serves it with webpack-dev-server


Thank you for the comment!

dsjen · 2016-09-20T19:56:29Z

webpack.config.js

-        alias: {
-            'edx-pattern-library': path.resolve(__dirname, 'pattern-library'),
-            'edx-ui-toolkit': path.resolve(__dirname, 'node_modules/edx-ui-toolkit/src')
+var devServerConfig = {


It seems like this could be declared with the other vars.

dsjen · 2016-09-20T19:56:57Z

webpack.config.js

+    }
+};
+
+var configFactory = function(options) {


Same declaration comment.

dsjen · 2016-09-20T19:58:30Z

webpack.config.js

-    siteRoot = process.env.SITE_ROOT !== undefined ? process.env.SITE_ROOT : '/',
-    publicJavaScriptRoot = 'public/';
+    webpack = require('webpack'),
+    ExtractTextPlugin = require('extract-text-webpack-plugin');


I think this comes from lack of knowledge on my part, and as a result, I would really appreciate some documentation about what this file is doing.

Added a bunch more documentation to this file about what all the different settings and paths are doing, let me know what you think.

dsjen · 2016-09-20T20:02:58Z

package.json

-    "development": "gulp build-development",
-    "lint": "eslint ."
+    "lint": "eslint .",
+    "start": "rm -rf pldoc/public && jekyll serve --incremental & webpack-dev-server && fg"


Nit: Is it possible to move this into a separate script? My general sense is that it's a bit lost in package.json.

"scripts": {} is a pretty common thing for a package to configure build/development tasks if it's not using another tool like gulp, so I guess the question is does having an npm start command that does the same thing as $(npm bin)/gulp add any value?

(I added this because I'm not a huge Gulp fan and I kind of just wanted to prove we didn't need it to do the development workflow... so having proved my point, I'll remove this and just make it an alias on my machine if nobody but me is going to use it 😛)

@dsjen I removed this, as I decided providing two paths to the same functionality isn't good design.

dsjen · 2016-09-22T19:51:12Z

@bjacobel -- thank you for responding to my feedback! I'm running the page using webpack and it's looking great for the most part. I did run into a problem where the page returns a 404 for http://localhost:8080/public/pattern-library-doc.css with the default gulp command. The styles looked to be loading fine though. To generate pattern-library-doc.css, I ran a production build to generate the assets for the public directory. Any ideas?

bjacobel · 2016-09-22T19:58:58Z

@dsjen, yep, I talk about the 404 for the css file in the "Known Issues" section of the PR. We can look at fixing it in the future, but it'd require creating a new Jekyll theme, I think.

dsjen · 2016-09-22T20:01:57Z

@bjacobel -- I see! Thanks for the clarification. :) I guess a jira ticket would be nice to capture that so that we can address it later.

LGTM! 👍

bjacobel added 4 commits September 16, 2016 17:05

Simplify, cleanup, and fully invest in Webpack

314ffc8

Rename lint task

0052c91

Add back build-production task

5e58f3c

Fix preview and doc-build tasks

9bdab7f

andy-armstrong reviewed Sep 19, 2016

View reviewed changes

andy-armstrong approved these changes Sep 19, 2016

View reviewed changes

JSDoc-ify

a5c6e31

dsjen reviewed Sep 20, 2016

View reviewed changes

bjacobel added 3 commits September 20, 2016 16:58

Code review comments and better Webpack documentation

21e7f95

Remove gulp/config.js entirely

aa68f93

Remove npm start script that duplicated gulp functionality

de4c58e

bjacobel merged commit 8811932 into master Sep 22, 2016

bjacobel deleted the bjacobel/fix-css branch September 22, 2016 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify and improve UXPL development/build process #367

Simplify and improve UXPL development/build process #367

bjacobel commented Sep 19, 2016 •

edited

mention-bot commented Sep 19, 2016

andy-armstrong Sep 19, 2016

andy-armstrong Sep 19, 2016

andy-armstrong left a comment

roderickmorales commented Sep 20, 2016

dsjen left a comment

dsjen Sep 20, 2016

bjacobel Sep 20, 2016

bjacobel Sep 20, 2016

dsjen Sep 20, 2016

dsjen Sep 20, 2016

dsjen Sep 20, 2016

dsjen Sep 20, 2016

dsjen Sep 20, 2016

dsjen Sep 20, 2016

dsjen Sep 20, 2016

bjacobel Sep 20, 2016

dsjen Sep 20, 2016

bjacobel Sep 20, 2016 •

edited

bjacobel Sep 22, 2016

dsjen commented Sep 22, 2016

bjacobel commented Sep 22, 2016

dsjen commented Sep 22, 2016

		var branch = gitUtils.currentBranch(),
		previewBaseUrl = '/' + branch + '/';

Simplify and improve UXPL development/build process #367

Simplify and improve UXPL development/build process #367

Conversation

bjacobel commented Sep 19, 2016 • edited

Description

Known issues

Sandbox

Testing Checklist

Non-testing Checklist

Reviewers

mention-bot commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-armstrong left a comment

Choose a reason for hiding this comment

roderickmorales commented Sep 20, 2016

dsjen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjacobel Sep 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsjen commented Sep 22, 2016

bjacobel commented Sep 22, 2016

dsjen commented Sep 22, 2016

bjacobel commented Sep 19, 2016 •

edited

bjacobel Sep 20, 2016 •

edited