Skip to content
This repository has been archived by the owner on Mar 5, 2020. It is now read-only.

Enable minification for production #386

Closed
wants to merge 3 commits into from
Closed

Enable minification for production #386

wants to merge 3 commits into from

Conversation

gesa
Copy link
Contributor

@gesa gesa commented Mar 24, 2015

Fixes #140

The good news: when i test this locally (basically by adding these tasks back in to the gulp watch task) this works great. I'm about 99% certain this will be fabulous on S3—though the only real way to test that is to upload it to S3.

The bad news: the sourcemaps this creates will be relatively worthless. So much so that I seriously considered not even including them.

@toolness
Copy link
Contributor

thanks @gesa!

I was originally thinking we'd just use webpack to minify the JS--its sourcemaps will be as useful as they are now, but with the added bonus of minification. It would require a different strategy than you're employing though--I was thinking that rather than separate gulp tasks for minification, we'd have to pass a command-line argument into gulp, like --production, and then have our webpack config be altered based on that. What do you think?

@toolness
Copy link
Contributor

Oh, also, I'm not very familiar with the LESS middleware, but I'm wondering if there's a similar way to tell it to minify as well? That way we could preserve the sourcemap goodness on our CSS as well!

@gesa
Copy link
Contributor Author

gesa commented Mar 25, 2015

Just leaving a note here that on develop currently css sourcemaps do not work. So, that just became scope for this PR.

@gesa
Copy link
Contributor Author

gesa commented Mar 25, 2015

Okay! I solved the sourcemaps problem! They now work on local development servers and minified on production! @toolness ready for review.

@gesa
Copy link
Contributor Author

gesa commented Mar 25, 2015

i have NO IDEA why the build is failing. can't reproduce locally =/

@toolness
Copy link
Contributor

Oh I've actually run into this before! I believe it's due to gulp-community/gulp-less#140...

@toolness
Copy link
Contributor

O yea, upgrading to a newer version of gulp-less seems to fix things on my end.

diff --git a/package.json b/package.json
index 30687a7..515b5fe 100644
--- a/package.json
+++ b/package.json
@@ -12,7 +12,7 @@
     "gulp-jscs": "^1.4.0",
     "gulp-jshint": "^1.9.2",
     "gulp-jsmin": "^0.1.4",
-    "gulp-less": "^2.0.3",
+    "gulp-less": "^3.0.2",
     "gulp-minify-css": "^1.0.0",
     "gulp-plumber": "^0.6.6",
     "gulp-prettify": "^0.3.0",

@toolness
Copy link
Contributor

That said, I am getting this weird "Potentially unhandled rejection" warning when running npm test:

2015-03-25_8-41-00

@gesa
Copy link
Contributor Author

gesa commented Mar 25, 2015

Yeah I had to downgrade the version of gulp-less in order to get functional sourcemaps…

@gesa gesa assigned gesa and unassigned toolness Mar 25, 2015
@toolness
Copy link
Contributor

Ok, I've salvaged a good bit of this PR into e6c5ad9 and 71ee7e4, so I'm closing it.

@toolness toolness closed this Mar 26, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants