Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI compile speed improvements #4

Closed
wants to merge 8 commits into from
Closed

Conversation

cheapsteak
Copy link
Contributor

We were previously uglifying about 70k LOC of vendor dependencies, most of which already had minified versions.

This PR adds a processing step to the index.html to include non-minified versions of those scripts during development in <script> tags, and use the minified versions for dist so that they'd only need to be concat'ed and not uglified.

Uglify will now only run on our own scripts.

This is a rather large change, throwing this up on dev first to check if everything compiles fine and that the sizes are as expected

TODO:

The grunt workflow is currently a bit messy, some tasks compile into the source folder, and the dev and dist tasks differ quite significantly. Since the dev task uses the source folder as the working folder, any files (e.g. index.html) that need to be transformed will either need a separate source template file in the same directory that's explicitly gitignored (confusing and messy, edit: but doing this now), or have the transforms done in the server middleware (unexpected, uncommon, ~~ but unfortunately what I had to resort to ~~ edit: reverted, ends up being messier than the former option)

The dev and dist tasks should more closely resemble each other, with minification and file revisioning being the only differences. Source files should not be compiled back into the same folder (as the sass files currently are). Multiple temporary directories might be required for now as long as we continue using Grunt

<script src="bower_components/showdown/src/extensions/table.js"></script>
<script src="bower_components/x2js/xml2json.js"></script>
<script src="bower_components/showdown/<%= grunt.config('env') === 'production' ? 'compressed/Showdown.min.js' : 'src/showdown.js' %>"></script>
<script src="bower_components/showdown/<%= grunt.config('env') === 'production' ? 'compressed/extensions/table.min.js' : 'src/extensions/table.js' %>"></script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use <%= minExt %>?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nvm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this thing could be done transparently through the build no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean, grab me when you have a sec

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I thought one of the grunt tasks we use rewrites the html file and replaces the various scripts with a concatenated version. I was wondering if their was enough metadata in the bower dependencies to be able to do all this automagically.

Copy link
Contributor Author

@cheapsteak cheapsteak Jun 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is the grunt task - https://www.npmjs.com/package/grunt-usemin

Unfortunately neither bower nor npm requires that level of metadata, e.g. here's angular's bower.json
https://github.com/angular/bower-angular/blob/master/bower.json
Notice that it only has a main that points to the unminified file

We could write something ourselves, either by

  • option 1) creating a custom user-defined step for grunt-usemin, when given a *.js file, it will try to resolve a *.min.js instead, we could additionally provide it with lookup tables with explicit paths to the minified files of libraries that don't follow the conventional structure
  • option 2) fork the library and hardcode our usecase to the existing concat task. (The library's deprecated with few updates anyway).

@cheapsteak
Copy link
Contributor Author

cheapsteak commented Jun 30, 2016

An alternative - switch out our build process to use either webpack or browserify (I'd recommend webpack)

Currently we're using index.html as both a template file and a config file, and since the parser for the config file is not robust enough to support the things we need to do, it necessitates either #4 (comment) or the contents of the current PR which adds another layer to the build step into index.html

If we go with webpack, we can additionally incrementally increase the scope of its responsibilities, starting first with just scripts (by using its script loader, we can do this without refactoring existing js files to use commonjs), but it can eventually subsume the clean, compass, autoprefixer, usemin, copy, cssmin, uglify, and filerev tasks currently being handled by Grunt

@btiernay
Copy link
Contributor

Ya, webpack seems like a good way to go. I think we should target this for after survival plot.

@btiernay
Copy link
Contributor

btiernay commented Jul 9, 2016

⚠️ Conflicts alert!

@cheapsteak
Copy link
Contributor Author

That's odd, I pulled from develop and the conflict fixed itself with no intervention from me

@btiernay
Copy link
Contributor

Retest this please

@btiernay
Copy link
Contributor

What's the intention with this branch? Do we still want to merge? Or should we wait for a build chain revamp?

@cheapsteak
Copy link
Contributor Author

Let's close this

On Fri, Jul 22, 2016 at 5:42 PM, Bob Tiernay notifications@github.com
wrote:

What's the intention with this branch? Do we still want to merge? Or
should we wait for a build chain revamp?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAtaKNPwSO95mXxf77dGZtr0nIXFvWDrks5qYTlLgaJpZM4JBcCV
.

@btiernay btiernay closed this Jul 22, 2016
bwalsh referenced this pull request in ohsu-comp-bio/dcc-portal Sep 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants