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

Fixed #22: "grunt uglify" doesn't update code #23

Closed
wants to merge 1 commit into from
Closed

Fixed #22: "grunt uglify" doesn't update code #23

wants to merge 1 commit into from

Conversation

madarche
Copy link
Contributor

No description provided.

@pimterry
Copy link
Owner

Thanks! I'm not sure about this though. A couple of points:

  • The reason this isn't updating your minified build is because you're not quite running it as intended. This is actually the standard Grunt setup for doing this (see http://gruntjs.com/sample-gruntfile, which has a matching worked example, also using concat.dist.dest), and if you run the default grunt task for this project (i.e. just run grunt), it'll run concat and then run uglify afterwards, so the ugilfy will minify the fresh concat output, and the result is correctly up to date. As a rule you typically want to run an explicitly defined grunt task, rather than calling plugins directly.
  • You do have a good point, in that this does go against expectations somewhat, and it's quite a bad result to end up with (the wrong code, minified).
  • At the same time though this fix will mean we instead produce minified source without the banner, which is not great for general project builds: having the banner is useful so you can check the version, or work out where what all this minified source is 6 months later. I don't think there's actually any correct output you can produce here while also keeping the concat setup in place as is. What I'd prefer is if I could have grunt enforce that you can't run uglify without running concat first, or just make running uglify directly impossible (or warned against). Any idea how/if I can do that?
  • Either way, in the meantime I'm going to add a note to the documentation describing how to build the project for distribution, and an explicit grunt dist task, which might hopefully make this a little clearer.

Oh, one other note: there's generally no need to make an issue and a matching pull request, unless you want to discuss something as an issue and only maybe do a pull request to fix it further down the line, or your PR is only a partial fix, or similar. Pull requests are just issues with code attached, and creating both when the code is really directly tied to the issue (as here, and most small fixes) is just a little inconvenient and messy for the person running the repo. Not a big issue, just something to watch for!

@madarche
Copy link
Contributor Author

@pimterry Thanks for the lengthy explanation and excuse me please. Makes me feel bad to have made you waste your time :-\ because I haven't use Grunt much so far. I have taken good notes of all :-) And it works all very well following your instructions.

I was expecting to find explanations of building the minified version of loglevel in the section where the use of node, Bower and JamJS is listed. So in the section "Downloading loglevel" maybe writing "If you're using node, you can run npm install loglevel. See section 'Contributing & Developing' for producing builds with grunt."

Best,

@pimterry
Copy link
Owner

pimterry commented Aug 3, 2013

No problem @madarche; I've added more text along those lines, and a proper grunt dist task to explicitly do this, which should make it a bit clearer in future. Thanks for filing this!

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