Minify static content by transpiling it first #421
Conversation
| var CleanCss = require('clean-css'); | ||
| var uglifyJs = require('uglify-js'); | ||
|
|
||
| module.exports = function(fileType, fileContent){ |
There was a problem hiding this comment.
this should be fileExt correct?
| minified = minifyFile('.js', content); | ||
| }); | ||
|
|
||
| it('should minify it', function(){ |
There was a problem hiding this comment.
a snapshot test, in this case, would simplify what you need to assert in order to make sure that the minification happened.
There was a problem hiding this comment.
With a snapshot I would test that
const hi = (name) => `hello ${name}`;would always translate to
var hi=function(n){return"hello "+n};Which is fine in this scenario, but what if at some point I upgrade babel and this starts translating to
var hi=function(a){return"hello "+a};So, my thinking was that I wanted to test something without taking the risk of coupling it too much to the snapshot. So I guess that was by design. Perhaps I'm overthinking this - as I see the benefit of seeing the simplicity of A to B transformation for a test readability point of view.
What do you think @nickbalestra and @mattiaerre ? Perhaps I don't know so much about babel + uglify internals. I tried doing a console.log of the process for 100 times and always gave back the same result.
There was a problem hiding this comment.
I think that the process is referentially transparent , so it should return always the same result
There was a problem hiding this comment.
ok, so let me update this. We can also change it in the future if this happens to be a problem.
There was a problem hiding this comment.
I personally think we should not test how the internal minification library works; that's why I was proposing a snapshot test. You will end up having a file containing your uglified code in one place. which is more or less what you do have now. What about a pairing session w/ you after this PR has been merged?
// cc @matteofigus @nickbalestra
| @@ -64,7 +34,7 @@ var copyDir = function(params, cb){ | |||
|
|
|||
| if(params.minify && params.ocOptions.minify !== false && (fileExt === '.js' || fileExt === '.css')){ | |||
| var fileContent = fs.readFileSync(filePath).toString(), | |||
| minified = minifyFile(fileExt, fileContent, params.ocOptions); | |||
| minified = minifyFile(fileExt, fileContent); | |||
There was a problem hiding this comment.
this is the only consumer of minifyFile I presume.
| return new CleanCss().minify(fileContent).styles; | ||
| } | ||
|
|
||
| return fileContent; |
There was a problem hiding this comment.
you shouldn't need to return here as the file extension check is done also at a higher level.
| './package-static-files': sinon.stub().yields(null, 'ok'), | ||
| './package-template': sinon.stub().yields(null, { type: 'jade', src: 'template.js', hashKey: '123456'}) | ||
| }, { __dirname: '' }); | ||
| }; |
There was a problem hiding this comment.
I see a lot of arrange before running this unit test. maybe this component does too many things?
There was a problem hiding this comment.
Agree. But I would separate in a different PR if you don't mind
There was a problem hiding this comment.
Sure, happy to pair on this if you like.
| extname: path.extname, | ||
| join: function(){ | ||
| return path.join.apply(this, _.toArray(arguments)).replace(/\\/g, '/'); | ||
| describe('cli : domain : package-static-files', function(){ |
There was a problem hiding this comment.
same as above re the arrange phase.
There was a problem hiding this comment.
Agree. That's why I am starting to take one piece off (minify part). Definitely gonna tackle other little pieces but I would do in separate PRs if you don't mind
|
@matteofigus this LGTM, happy to merge if you like. |
Fixes #409
Summary: #418 was opened and merge to fix #409 but we had to rollback as the packaging was producing an invalid bundle (not good for browsers and potentially fragile for uglify). So #420 reverted it. This should be finally the one, with more testing, more fixes etc.
To review, I recommend to go commit by commit.