breezewish / express-minify

Automatically minify and cache your javascript and css files.
https://npmjs.org/package/express-minify
MIT License
86 stars 18 forks source link

It's not working for me. #49

Closed nirsharma closed 7 years ago

nirsharma commented 8 years ago

this is my code app.use(minifying({ js_match : /\.js$/, css_match : /\.css$/ })) right before express static middleware.

  1. no js or css file is being minified.
  2. not getting any response to data APIs

I tried using compression, that works just fine. But minification , with or without compression is not working.

breezewish commented 8 years ago

Hi, could you try with just app.use(minifying())? js_match and css_match are matched with content-types, instead of urls.

nirsharma commented 8 years ago

I had tried with just app.use(minifying()) but it didn't work, files weren't loading

tayler-king commented 8 years ago

+1

Using App.use(require('express-minify')()); - not minifying any css or javascript files for me either.

Martii commented 8 years ago

It's working on our production vs. development... has anyone noticed that minifying should probably be minify ?

Also what version is your express?

npm update tried?

Is there any package in your project that modifies express's MIME content types from their standards? e.g. express.static.mime.define...

Ref(s):

nirsharma commented 8 years ago

@Martii

  1. const minifying = require('express-minify') so it shouldn't be a problem.
  2. express version is 4.14.0
  3. not even touching the MIME content types I am using it along with compression middleware which is working fine but not this one.
tayler-king commented 8 years ago

@nnIIInnjA Same with me, apart from not using the compression middleware.

I've tried using the middleware in every order combination possible - still no success.

Edit: Just took a look, and it seems to be minifying some files (some JS files), but not others (some JS files, CSS files). It's not showing any errors either.

Okay, seems to be an issue of files failing to minify but the onerror function not being called.

This file, for example, will not minify nor will it error:

var Routes = angular.module('routes', []);

Routes.config(['$routeProvider', function($RouteProvider){

    var $Route = $RouteProvider.$get[$RouteProvider.$get.length-1]({$on:function(){}});

    $RouteProvider
    .when('/duel', {
        templateUrl: 'partials/duel',
        controller: 'Duel',
        reloadOnSearch: false
    })
    .otherwise({
        redirectTo: '/duel'
    });

}]);

I've also found that using let (ES6/ES2015) will result in SyntaxError: Unexpected token: name ([Variable Name]) - I had to replace all let declarations with var.

UglifyJS doesn't officially support ES6/ES2016 - see https://github.com/mishoo/UglifyJS2/issues/448

Also just noticed, CSS files won't be minified if you have an @import statement in the CSS.

Although these aren't directly issues with this library, there should (from my point of view) have errors occur if these tasks fail.

Martii commented 8 years ago

@Kondax

UglifyJS doesn't officially support ES6/ES2016

That's why OUJS has forked the harmony branch to coincide with release that express-minify here uses. See our fork at https://github.com/OpenUserJs/UglifyJS2/tree/harmony%2Brenamed .

Also just noticed, CSS files won't be minified if you have an @import statement in the CSS.

That's #18 here.

I am using it along with compression middleware which is working fine but not this one.

We are too although I haven't checked the headers recently to ensure Gzippin is occuring... See https://github.com/OpenUserJs/OpenUserJS.org/blob/98c5b3f1ddbff33e2ab7e11052d97b894e4d1946/app.js#L272

EDIT: Retested PASS

GET /css/common.css HTTP/1.1
Host: openuserjs.org
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 SeaMonkey/2.40
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
DNT: 1
Connection: keep-alive

HTTP/1.1 200 OK
X-Powered-By: Express
Strict-Transport-Security: max-age=31536000000; includeSubDomains
Accept-Ranges: bytes
Cache-Control: public, max-age=86400
Last-Modified: Wed, 31 Aug 2016 19:46:17 GMT
Etag: W/"159e-156e2234328"
Content-Type: text/css; charset=UTF-8
Vary: Accept-Encoding
Content-Encoding: gzip
Date: Wed, 12 Oct 2016 02:02:25 GMT
Connection: keep-alive
Transfer-Encoding: chunked

In general it's quite picky getting the order right and we rarely mess with it. I just retested this yesterday because of this issue too. I'm usually watching for potential issues for our usage as well.

EDIT

there should (from my point of view) have errors occur if these tasks fail.

uglifyjs2 is the one that throws the errors not express-minify... they are just caught here usually.

@nnIIInnjA

const minifying...

Have you tried var... perhaps there's a node issue with ES6 let and const... don't know for sure... we are primarly ES5 with the minification as experimental right now with Userscripts since uglifyjs2 ES6 support is on a branch and not release... however our actual production is using release uglifyjs2.

breezewish commented 8 years ago

Yeah. The default error behavior is to return the original content (so that errors won't break anything) when there is a minifying error. https://github.com/breeswish/express-minify#customize-onerror-behavior

Since more and more people is using ECMAScript 2016+ which is not supported by uglifyjs currently, I would update the documentation to suggest users to add an error handling logic so that they will get noticed about this issue.

Martii commented 8 years ago

..add an error handling logic...

Yep that's here in ours from #44 here... Ace editor (ace-builds) sometimes, but not all the time, does throw one with release uglifyjs2. I've been watching that project too... haven't pinned down what Ace is doing and how to reproduce yet but it's only about twice a month.

nirsharma commented 8 years ago

@Martii I tried with var too. not working :( FYI m not using any import statement in my css, it's very basic plan css file. Also js code is transpiled down to es5 before server to fetch so that also shouldn't be a problem

tayler-king commented 8 years ago

@Martii ah right, makes sense. Maybe an issue will have to be opened for UglifyJS2 not throwing errors correctly.

Well, at least everything apart from CSS seems to be minified for the most for me. CSS randomly works or not - but that library is super old.

Martii commented 8 years ago

@nnIIInnjA Is your transpiler middleware too? e.g. what is it on npmjs.com and I can try to give it a test whirl in our development. We're using the same version of express as you... we're also still on node 4.x latest but development here on my dev station is node@6.x and it still returns the same results when I kick dev into ignoring dev vs pro and debug modes. Better question is your project open source so I can try it out with a git checkout?

EDIT Do you get any errors with $ npm install btw? Any binary compiling issues?

@Kondax

Maybe an issue will have to be opened for UglifyJS2 not throwing errors correctly.

They currently allow "downstream" or "across the pond" to handle errors... express-minify catches them and allows additional processing to maintain site integrity... you wouldn't want a site just erroring out and having to restart the process(es). Proposing that would be a breaking change in many, many projects that utilize uglifyjs2 though... not to mention the CLI. I linked in our error handler to show how to put in a handler for anyone watching including you... I may bump our body value up to 200 characters as it took me a few months to track down that Ace had an intermittent glitch on occasion but if it doesn't minify then that's why I'm over there to report issues. We have release (master) and harmony packages/git in order to help improve it... anyhow... it's up to you but breaking changes can be an issue in itself over there. They seem very receptive and I know how long development really takes especially in a modular system like node/npm.

but that library is super old.

Which library now?

nirsharma commented 8 years ago

@Martii Sadly my project isn't open source and nope transpiling is done beforehand and not in express. I've also checked error while npm install none this is the code :- const h2 = require('http2') const fs = require('fs') const config = require('config') const path = require('path') const httpProxy = require('http-proxy') const express = require('express') const R = require('ramda') const request = require('request') const cookieParser = require('cookie-parser') const gZipping = require('compression') const minify = require('express-minify') const app = express() app.use(cookieParser())

express.request.__proto__ = h2.IncomingMessage.prototype; express.response.__proto__ = h2.ServerResponse.prototype;

// to gZip files app.use(gZipping())

// to minify files app.use(minify())

// to serve static files app.use('/', express.static(__dirname))

there's more code here as well but that is mostly to handle data apis and nothing to do with files

nirsharma commented 8 years ago

@Martii Update : I've tried hitting just to get the styles.css file and i got this :- Resource interpreted as Document but transferred with MIME type text/css: "https://localhost:5555/styles.css".

Martii commented 8 years ago

@nnIIInnjA After I built a basic package.json for your requires... and added in a listening port of 3000 at the tail of app.js... it's minifying our common.css copy here in node@6.x. :\

package.json

{
  "name": "Test",
  "description": "Test",
  "version": "0.0.0",
  "main": "app",
  "dependencies": {
    "http2": "3.3.6",
    "config": "1.21.0",
    "http-proxy": "1.15.1",
    "express": "4.14.0",
    "ramda": "0.22.1",
    "request": "2.75.0",
    "cookie-parser": "1.4.3",
    "compression": "1.6.2",
    "express-minify": "0.2.0"
  },
  "repository": {
    "type": "git",
    "url": "https://github.com/nnIIInnjA/test"
  },
  "license": "(GPL-3.0 AND GFDL-1.3)"
}

app.js

const h2 = require('http2')
const fs = require('fs')
const config = require('config')
const path = require('path')
const httpProxy = require('http-proxy')
const express = require('express')
const R = require('ramda')
const request = require('request')
const cookieParser = require('cookie-parser')
const gZipping = require('compression')
const minify = require('express-minify')
const app = express()
app.use(cookieParser())

express.request.proto = h2.IncomingMessage.prototype;
express.response.proto = h2.ServerResponse.prototype;

// to gZip files
app.use(gZipping());

// to minify files
app.use(minify());

// to serve static files
app.use('/', express.static(__dirname));

app.listen(3000);

http://localhost:3000/common.css

Only thing that I can think of is maybe your platform and or node is out of date. Did you build your own node or use someones prepackage? What is your platform btw?

nirsharma commented 8 years ago

@Martii node version :- 6.6.0 MacOs Sierra

Martii commented 8 years ago

OS X is the one platform I haven't tried our project on... what is your current Xcode version? Linux here.

nirsharma commented 8 years ago

@Martii I haven't installed xcode. I don't see how that's relevant to our issue. OS version :- macOS Sierra 10.12

Martii commented 8 years ago

@nnIIInnjA

I haven't installed xcode. I don't see why that's relevant to our issue.

If you build node it is. Didn't get an answer if you prepackaged or not. They are on 8 on El Capitan... and it's quite relevant as there many issues. I'm not ready to upgrade my Mac to Sierra yet for a month or more as Apple has too many issues with Xcode right now in El Capitan. Anyhow... your snippet works on Linux so I'm not entirely sure what the deal is.

nirsharma commented 8 years ago

@Martii ohh sorry, i missed there. I am not building node, it's prepackaged.

Martii commented 8 years ago

@nnIIInnjA Homebrew (which says 6.2.1 here on my Mac but it's El Capitan build) or direct image from node ? You are one version behind the latest release at nodejs.. I'm at 6.7.0 on Linux.

Resource interpreted as Document but transferred with MIME type text/css: "https://localhost:5555/styles.css".

With this your MIME types might be foo'd or set incorrectly... http://www.rubicode.com/Software/RCDefaultApp/ , or equivalent, should at least be able to tell you if the OS itself is set correctly with those MIME types but that doesn't exclude if node is built incorrectly or for an older OS X.

nirsharma commented 8 years ago

@Martii i've had older version of node sometime back 6.1.0 which i installed using direct image of node. recently i updated to 6.6.0 using brew.

I am using http2 module. would that be an issue ?

Martii commented 8 years ago

@nnIIInnjA Okay after I installed brews version onto El Capitan and using the coded snippets above it is minifying our common.css.

Sooooooooooo... at least El Capitan is confirmed to work with node@6.2.1 and express-minify@0.2.0 here. My guess it's something in Sierra and/or the built package of node on brew (they call them bottles here on my Mac)... I would suggest investigating if your 6.6.0 is for sure Sierra compatible and make sure OS X is up to date. I also have the "Command Line Tools" from the App store installed along with Xcode for another project.

nirsharma commented 8 years ago

@Martii I just tried with http1 , it seems to work fine. I think the issue is with http2 which i am using in my project

tayler-king commented 8 years ago

@Martii In regards to error handling, I mean if something for some reason fails to minify. I dumped your error handler into my application to see if it would help - still only errors randomly.

CSSMin is the library that seems rather outdated - no updates for a few years. That's what randomly works for minifying my CSS.

Martii commented 8 years ago

@Kondax

CSSMin is the library that seems rather outdated

Well if you see an issue might want to file it over there. With as fast as everything changes sometimes it's good to be reminded if something isn't working in a dependency.

still only errors randomly

If you can pin it down to a particular rule set file a bug report on his site. :) All of our CSS seems to be working and never had a CSS glitch in our logs yet... just JavaScript every once in a while for production. We even use LESS compiling to CSS and that's been good too.

tayler-king commented 8 years ago

@Martii well, when I say random I honestly mean it. Without changing any of my application or the files to be minified, sometimes they will be minified and sometimes not. No idea how to track the cause of that.

tayler-king commented 8 years ago

Still seems to be icky for me - not minifying certain files, not erroring either. If I minify the files directly through uglifyjs2, it works. Weird.

melroy89 commented 7 years ago

Use minify, and use var instead of const? Did you set the static folder, the folder where the js/css files should be located, to in my case the 'public' folder? Keep it simple first.

var minify = require('express-minify');
app.use(minify());
app.use(express.static(path.join(__dirname, 'public')));

I'm using express-minify 0.2.0 and expressjs version 4.13.4, all working here.

tayler-king commented 7 years ago

Not if I named the variable minifying. On Thu, 3 Nov 2016 at 4:31 pm, Melroy van den Berg notifications@github.com wrote:

Wait wait, you use minifying? The command is without 'ing':

app.use(minify());

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/breeswish/express-minify/issues/49#issuecomment-258196613, or mute the thread https://github.com/notifications/unsubscribe-auth/AQFo-nWOU54AeNff29KuA0ToaYwK8FSIks5q6gxNgaJpZM4KSF68 .

melroy89 commented 7 years ago

Noticed, I changed my comment view the webpage plz

On Nov 3, 2016 18:54, "Tayler King" notifications@github.com wrote:

Not if I named the variable minifying. On Thu, 3 Nov 2016 at 4:31 pm, Melroy van den Berg < notifications@github.com> wrote:

Wait wait, you use minifying? The command is without 'ing':

app.use(minify());

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/breeswish/express-minify/issues/49# issuecomment-258196613, or mute the thread https://github.com/notifications/unsubscribe-auth/AQFo- nWOU54AeNff29KuA0ToaYwK8FSIks5q6gxNgaJpZM4KSF68 .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/breeswish/express-minify/issues/49#issuecomment-258222352, or mute the thread https://github.com/notifications/unsubscribe-auth/AAmYvnFPNU40ZrkHCy9sI-RSDjjS36cBks5q6h_LgaJpZM4KSF68 .

tayler-king commented 7 years ago

Variable name doesn't change anything, nor does switching between the type of variable.

Static folder is of course set. I've switched to webpack anyhow now, since this is just annoying.

melroy89 commented 7 years ago

I understand, strange I can't not reproduce the issue yet. Since in my case it just works.. Could it be owner/privileges rights problem? Read & write.

tayler-king commented 7 years ago

Nope, has all the privileges needed. On Fri, 4 Nov 2016 at 1:10 pm, Melroy van den Berg notifications@github.com wrote:

I understand, strange I can't not reproduce the issue yet. Since in my case it just works.. Could it be owner/privileges rights problem? Read & write.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/breeswish/express-minify/issues/49#issuecomment-258426161, or mute the thread https://github.com/notifications/unsubscribe-auth/AQFo-ivjENHx2fC4M9xTO1Suut6hz9_Nks5q6y45gaJpZM4KSF68 .

breezewish commented 7 years ago

I'm going to add some debug output to ease the debugging.

martinlevesque commented 7 years ago

js/css were not minifying also for me. I have an nginx serving my node application and static files were served by nginx:

location ~ \.(html|js|css|gif|jpg|png)$ {
   root ...
}

To make js/css minified I changed it to

location ~ \.(html|gif|jpg|png)$ {
   root ...
}
breezewish commented 7 years ago

@martinlevesque Yes you have to use express to serve static files otherwise it won't work.

breezewish commented 7 years ago

Closed due to no activity for long time. Please open another PR if you still meet issues.

searene commented 6 years ago

One of my js files were not minified, either. After some experiments, I found out that the reason why it was not minified was that it used arrow functions. It worked after I changed it to a normal function.

Martii commented 6 years ago

@searene Cc: @breeswish

I found out that the reason why it was not minified was that it used arrow functions

It worked after I changed it to a normal function.

From experience the arrow operator doesn't offer any perceived enhancement and actually is a watered down version of function as you figured out.

LSafer commented 3 years ago

How could I utilize the uglifyJSModule option ? Should I make a custom implementation? Or is there a reference that I should pass as an option. Since the express-minify rejected mapping the option uglifyJSModule to the exported object from terser