Make express-minify optional for dev #431
Conversation
|
How about we not invite production to potentially be misconfigured like your machine is and propagate that error by keeping the |
|
Just for grins here's my output if it will help you or anyone else: $ npm install express-minify
npm WARN package.json bootstrap-markdown@2.7.0 'repositories' (plural) Not supported. Please pick one as the 'repository' field
npm WARN package.json mu2@0.5.20 No repository field.
-
> node-sass@0.9.6 install ~/repo/git/oujs/martii/OpenUserJS.org/node_modules/express-minify/node_modules/node-sass
> node build.js
`linux-x64-v8-3.14` exists; testing
․․․․․․․․․․․․․․․․․․․․․․․․․․
26 passing (28ms)
Binary is fine; exiting
express-minify@0.0.11 node_modules/express-minify
├── on-headers@1.0.0
├── cssmin@0.4.2
├── coffee-script@1.8.0 (mkdirp@0.3.5)
├── uglify-js@2.4.15 (uglify-to-browserify@1.0.2, async@0.2.10, optimist@0.3.7, source-map@0.1.34)
├── stylus@0.47.3 (css-parse@1.7.0, mkdirp@0.3.5, sax@0.5.8, debug@2.1.0, glob@3.2.11)
├── less@1.7.5 (graceful-fs@3.0.4, mime@1.2.11, source-map@0.1.40, mkdirp@0.5.0, clean-css@2.2.18, request@2.40.0)
└── node-sass@0.9.6 (get-stdin@3.0.0, object-assign@1.0.0, node-watch@0.3.4, node-sass-middleware@0.3.1, nan@1.3.0, mkdirp@0.5.0, shelljs@0.3.0, yargs@1.3.3, chalk@0.5.1, sinon@1.10.3, mocha@1.21.5) |
|
If this removes minification in production, it'll be because nodejitsu doesn't install optionalDependencies, which is possible. I googled this first and found this (nodejitsu/require-analyzer#48). Even if this is the case, it's not going to break the site, it'll just turn off minification temporarily. If this is the case, it's ridiculously easy to test and confirm. And a quick fix after the fact to stuff it back under the dependencies list. if (isPro) {
var minify = require('express-minify');
app.use(minify());
} else {
try {
var minify = require('express-minify');
app.use(minify());
} catch (e) {}
}Good enough? |
Almost...
What's that saying... don't ASSume... there are exceptions to the current top inclusion for Anyhow... if you correct the logic for debug mode (or provide a good reason why we should just apply express-minify all across the board)... I'll give it a thumbs up... as it stands right now it's a thumbs down since you changed the logic for personal reasons. |
... now that you've edited your post above and removed what you said about me complaining... you now have a strict mode violation of redeclaring |
Ah yes. Debugging with unminified code helps. Except chrome (and probably firefox) can unminify/prettyprint it for you. Don't really see why debugging the NodeJS v8 runtime helps you debug responses but hey, I'll run with it. Fixed the PR. |
I see that at Zren/OpenUserJS.org@66a7d9c...ad649d0 however I don't see it here. Also can we please keep the Ace comment note in... I don't know if it's a stale test result by now but that's kind of a |
As a FYI... That's great that client sides can do this... but not all debugging comes from client side reports... how do you think I resolved #395 so quickly? |
Reload the page.
Done. And since we've established this is done... Merging. |
Make express-minify optional when isDev
GH shows that you didn't do it until just recently... not to mention those are ajaxed in so there is no need for a page refresh.
LOL... don't get in trouble with sizzle here... he's temporarily left merging up to me until he says differently... so please respect his wishes... I do... so should you. |
|
You also ignored the PR READY label and categorization too... but since I'm here we'll let it slide this time. |
Martii
added
CODE
|
Ah? Figured I was allowed to merge myself since I was added to the OUJS group (not that I have)... but alright. |
|
Part of project administration is working with each other... sizzle initially didn't mind anyone merging but then they were too fearful to merge things... so then he took over full time on it... then he asked me to take over for a while... this could change the more we all communicate effectively together... I'd prefer the thumbs up thing and +1 and also leaving things to be discussed with other original founders... I will however ask questions when I feel something might be a concern myself as should you... but I'm going to try not to make things personal and keep it on a professional level. We all have our strengths and weaknesses... I've learned a great deal from you and I do appreciate that and I hope that my sharing has enlightened some others including you as well. |
|
One last diddy... did |
|
No, because it's not optional on production and didn't feel like dealing with it. |
|
Alright thank you. |
|
Change looks good to me. I remember saying this somewhere else, but when one person submits a PR, someone else should merge it when a consensus has been reached. The only exception is the active maintainer (because some stuff just needs to get done without waiting for input), who is Martii right now. |
|
I should give some fair warning to everyone especially @Zren ... I just reread express-minify README.md and that project claims to compile |
* Apparently order is important again here and OpenUserJS#431 didn't take into account this by skipping regression testing * Alter the routine to restore the order and the exception of not using `require` at the top is rescinded * `try..catch` is still used for those devs with misconfigured environments
Reapplies to: * OpenUserJS#434 * OpenUserJS#431 Eventually this test should go away when @Zren takes the necessary time to reinstall his Windows machine instead of making a personal exception since his machine is misconfigured.
|
Looks like I will be putting express-minify in optionalDependancies as node-sass does not build on heroku (linux-x64 / Node 0.10.33 / NPM 1.4.28) either. I'm not the only one getting this. See: /sass/node-sass/issues/550 |
|
I think we need to do some diags here... what's your distro name? I'm already getting latest Ubuntu... 12 seems pretty old as mentioned over there. |
|
99% sure it works for you because you have npm 2.x |
Does |
Nope... sass/node-sass#550 See also: |
Martii
changed the title
No description provided.