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

Issue102 #139

Closed
wants to merge 5 commits into from
Closed

Issue102 #139

wants to merge 5 commits into from

Conversation

alicoding
Copy link
Collaborator

This patch should fix the issue we are having in #135 as well.

Right now by default when you run gulp dev or npm start it shouldn't uglify the JS for you since it's for development. The downside here as we know it will slow down the browser load time, but obviously it will increase our productivity significantly.

We still have the useful webpack plugin enabled when you run gulp only which should be the build step for us on production.

I'm still having trouble getting LESS to work on Firefox for some reason but other than that this is in a good state at the moment.

@alicoding alicoding force-pushed the issue102 branch 2 times, most recently from aec318e to 49b29dd Compare March 4, 2015 22:08
@alicoding
Copy link
Collaborator Author

@toolness R?

@toolness
Copy link
Contributor

toolness commented Mar 4, 2015

Awesome thanks Ali!

Hmm, I am seeing the sourcemaps for the CSS/LESS, but not for the JS... When I intentionally add an exception in the code, for instance, it just shows me bundle.js rather than the original file it came from in Chrome:

screen shot 2015-03-04 at 6 04 55 pm

Loading bundle.js in my text editor, I'm also not seeing any mention of sourceMappingURL like I see in styles.css...

@toolness
Copy link
Contributor

toolness commented Mar 4, 2015

Oh, so I'm now noticing that when I run gulp, a source map for bundle.js is being generated, but bundle.js isn't being uglified.

And when I run npm start (aka gulp dev), no source map for bundle.js is generated, nor is bundle.js uglified.

So I guess things need to change so that the bundle is uglified for production, and source maps are generated for development?

This is getting a little confusing--I'm starting to think we need a test suite for our own build system! 😅

var outputPath = path.join(__dirname, COMPILED_DIR, 'js');
fs.removeSync(outputPath);
var outputName = 'bundle.js';
var config = require('./webpack.config');
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, since we are essentially dynamically creating our webpack config, perhaps we should just do away with webpack.config.js entirely?

@toolness
Copy link
Contributor

toolness commented Mar 5, 2015

So this PR is doing lots of stuff that I'm confused about... I think it might be better to split this into two different objectives:

  1. Adding JS and CSS source maps to all builds, regardless of whether they're dev or prod. Since the source maps can be in a separate file that only devtools access, they shouldn't impact the load times of production builds, and they also encourage anyone to delve into our site and see how it works.
  2. Uglifying JS and CSS output for production builds only.

Is it ok if I take the very basics of what you've done to address (1) and issue a really simple separate PR to address them? Then we can address (2) anytime before launch.

@alicoding
Copy link
Collaborator Author

Closing this in favour of #147

@alicoding alicoding closed this Mar 5, 2015
@alicoding alicoding deleted the issue102 branch March 5, 2015 19:48
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