Navigation Menu

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

Refactor asset management #1813

Closed
wants to merge 18 commits into from
Closed

Refactor asset management #1813

wants to merge 18 commits into from

Conversation

aron
Copy link
Contributor

@aron aron commented Dec 17, 2014

This pull request aims to refactor our asset bundles to allow us to produce both non-minified and minified production assets depending on the use case. This was required for the Firefox addon where the moderators prefer assets to be non-minified so that they are easier to review.

The main change here is to move away from having our assets declared in a yaml format to using python to build the bundles (asset_bundles.py). I think for a small project the yaml makes sense, but our current file is extremely verbose and full of duplication making it hard to understand.

This change tries to ensure dependency definitions are just filename strings. A create_bundle helper (asset_helpers.py) then works out whether the file is CoffeeScript or SCSS based on the file extension and compiles it accordingly. This has the added benefit of giving us a 1-1 mapping of coffee to js files in development.

Bundles are then registered with the application and given production file names in a second step at the bottom of asset_bundles.py. This again checks to see if a new hassets.minify setting is true and then applies the minification filters and a .min file extension.

The result gives us a cleaner file that clearly defines the dependencies for each file and removes all the boilerplate around minification and compilation.

Other small changes include...

  • moving the production output to static/build and development debug output to static/debug. This keeps our actual source files separate from generated output and simplifies our clean scripts.
  • I removed a lot of outdated and unused code in scripts.py that was copying old deleted assets.
  • Firefox assets are no longer minified.
  • Included non minified and updated versions of jQuery and Katex.
  • Moved bootstrap.js into scripts/
  • Concatenates all core angularjs dependencies (in "app" bundle) into a single file

@@ -23,6 +25,8 @@ module.exports = function(config) {
'h/static/scripts/vendor/angular-resource.js',
'h/static/scripts/vendor/angular-route.js',
'h/static/scripts/vendor/angular-sanitize.js',
'h/static/scripts/vendor/angular-bootstrap.js',
'h/static/scripts/vendor/angular-resource.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is here twice now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both -bootstrap and -resource.

@tilgovi
Copy link
Contributor

tilgovi commented Dec 17, 2014

webassets.debug is functionally already doing what the hassets.minify flag you added does. You can even create the bundles with the minifiers but pass debug=True if you really want to do it in Python rather than YAML. There's no need to remove them from the pipeline.

We used to have python-defined bundles. I moved them to YAML because I thought it was actually clearer that way.

@tilgovi
Copy link
Contributor

tilgovi commented Dec 17, 2014

e2e7a16

@aron
Copy link
Contributor Author

aron commented Dec 17, 2014

webassets.debug=False doesn't quite do what I want. It bypasses the minification and compiles all the coffee and scss files. But the downside is that in order to get the unminified production files we need to concatenate the app.js, hypothesis.js etc files in development too.

This PR completely separates the two. We now have a 1-1 map between coffee and js files during development. And one set of production files that are explicitly built either minified or not. I know it's annoying having an extra setting, but at least this way minification is an optional part of the production build process.

I think for a smaller project the YAML file is definitely clearer but when we have to write four lines of YAML to declare a single CoffeeScript dependency I think its time to use a script to reduce that boilerplate. I also found it difficult to work out which bundles were actually used in production when all the dependencies have output and filters: uglify properties even though they're just bundled together further down the file.

@tilgovi
Copy link
Contributor

tilgovi commented Dec 17, 2014

You can specify debug: true or debug: false for any bundle, I think, to force it. Precompilers like coffee are executed regardless. So you could bypass uglify but still concatenate, if that's what you're wanting?

@tilgovi
Copy link
Contributor

tilgovi commented Dec 17, 2014

As for the lots and lots of uglifying, I think some of that is a remnant of having once distributed many of these separately. Now https://hypothes.is/app.html only shows jquery, angular, angular-animate, angular-route, and angular-sanitize (in addition to app, helpers, session, and account). So we could probably just refactor the YAML to avoid minifiying a lot of things before concatenation, and then minify them all together.

@aron
Copy link
Contributor Author

aron commented Dec 17, 2014

I'm not sure how passing debug individually helps here. As for re-factoring the YAML, we could just define bundles for the production assets but that would mean that during development we would be working with those same files unminified.

We could do this, but I like having the individual files during development, it makes working with the codebase more pleasant than doing searches through tens of thousands of lines of concatenated JavaScript for your current file.

@tilgovi
Copy link
Contributor

tilgovi commented Dec 17, 2014

Right, sorry. Of course, we wouldn't want to concatenate all the time so maybe that comment wasn't useful.

But we can avoid a lot of the boilerplate you're trying to avoid for those assets that we never ship individually anyway. All those annotator plugins don't need minified targets if we minify them in the same stage as the concatenation. Same with all the momentjs and other small vendor libs that just get bundled at the end anyway.

Other than that I'm not seeing a ton of boilerplate and I find the YAML much more readable.

What this PR does accomplish that I like is that each coffee file has a separate js output. In terms of usefulness during debug this could be accomplished just as well by creating a filter that builds the coffee with inline source maps. For instance, I would be very happy if we could define the application bundles with just the entry point .coffee and a browserify filter.

In the meantime, I think this is a very marginal improvement and it will probably cause some pain downstream in webplatform/annotation-service or any other project that wants to override bundles but re-use our yaml. I added support to pyramid_webassets for multiple files in the webassets.bundles setting with cascading overrides just for this kind of thing.

@aron
Copy link
Contributor Author

aron commented Dec 18, 2014

Okay, I hear you, I'll cherry pick out the useful parts of this PR and start again.

@aron aron removed the Needs Review label Dec 18, 2014
@aron
Copy link
Contributor Author

aron commented Dec 19, 2014

I'm closing this. I looked into browserify yesterday, I think we can achieve the same thing.

I actually think the simpler way to do this would be to have a make task or similar to actually build the assets and then just define the primary bundles in the assets.yaml. The combination of coffee/js and non-broweserifyable assets (or at least not without a shim) makes the build process more complex.

The other way I can see to do it would be to browserify our application code and bundle all vendor dependencies in separate files. Then we'd just need to work out how to pass exclude flags into a browserify webassets filter.

Would be good to talk this through with you as I've little idea of the requirements of the downstream projects.

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.

None yet

2 participants