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.

Sign up for GitHub

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

Jump to bottom

Minify static content by transpiling it first #421

Merged
merged 8 commits into from
Mar 27, 2017
Merged

Minify static content by transpiling it first #421

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d50cc5e
Minification back with uglify option
matteofigus Mar 24, 2017
7083fda
Minify bugfix
matteofigus Mar 25, 2017
d021498
Test babel is called with correct parameters
matteofigus Mar 25, 2017
0b0e461
Moved things around
matteofigus Mar 25, 2017
ca995d7
Consistency
matteofigus Mar 25, 2017
9e89afb
A couple of integration tests
matteofigus Mar 25, 2017
6c08f08
Requested changes
matteofigus Mar 27, 2017
d63c6e0
Simplified test
matteofigus Mar 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cli/domain/package-components.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ module.exports = function(){
return function(options, callback){

var componentPath = options.componentPath;
var minify = options.minify || true;
var minify = options.minify === true;

var files = fs.readdirSync(componentPath),
publishPath = path.join(componentPath, '_package');
Expand Down
36 changes: 3 additions & 33 deletions src/cli/domain/package-static-files.js → src/cli/domain/package-static-files/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,44 +1,14 @@
'use strict';

var async = require('async');
var babel = require('babel-core');
var babelPresetEnv = require('babel-preset-env');
var CleanCss = require('clean-css');
var format = require('stringformat');
var fs = require('fs-extra');
var minifyFile = require('./minify-file');
var nodeDir = require('node-dir');
var path = require('path');
var uglifyJs = require('uglify-js');
var _ = require('underscore');

var strings = require('../../resources');

var minifyFile = function(fileType, fileContent, ocOptions){

if(fileType === '.js'){
/*
2017-02-24 Reverting #418

var presetOptions = {
targets: {
browsers: 'not ie <= 8'
}
};

var babelOptions = {
presets: [[babelPresetEnv, presetOptions]]
};

var es5 = babel.transform(fileContent, babelOptions).code;

*/
return uglifyJs.minify(fileContent, { fromString: true }).code;
} else if(fileType === '.css'){
return new CleanCss().minify(fileContent).styles;
}

return fileContent;
};
var strings = require('../../../resources');

var copyDir = function(params, cb){
var staticPath = path.join(params.componentPath, params.staticDir),
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

this is the only consumer of minifyFile I presume.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes


fs.writeFileSync(fileDestination, minified);
} else {
Expand Down
32 changes: 32 additions & 0 deletions src/cli/domain/package-static-files/minify-file.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
'use strict';

var babel = require('babel-core');
var babelPresetEnv = require('babel-preset-env');
var CleanCss = require('clean-css');
var uglifyJs = require('uglify-js');

module.exports = function(fileType, fileContent){
Copy link
Member

Choose a reason for hiding this comment

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

this should be fileExt correct?


if(fileType === '.js'){

var presetOptions = {
targets: {
browsers: 'ie 8',
uglify: true
},
useBuiltIns: true,
modules: false
};

var babelOptions = { presets: [[babelPresetEnv, presetOptions]] },
es5 = babel.transform(fileContent, babelOptions).code;

return uglifyJs.minify(es5, { fromString: true }).code;

} else if(fileType === '.css'){

return new CleanCss().minify(fileContent).styles;
}

return fileContent;
Copy link
Member

Choose a reason for hiding this comment

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

you shouldn't need to return here as the file extension check is done also at a higher level.

};
39 changes: 39 additions & 0 deletions test/integration/cli-domain-package-static-files-minify-file.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';

var expect = require('chai').expect;

describe('cli : domain : package-static-files : minify-file', function(){
var minifyFile = require('../../src/cli/domain/package-static-files/minify-file');

describe('when minifying .js file', function(){

describe('when file contains es6', function(){

var content = 'const hi = (name) => `hello ${name}`;';
var minified;

before(function(){
minified = minifyFile('.js', content);
});

it('should minify it', function(){
Copy link
Member

Choose a reason for hiding this comment

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

a snapshot test, in this case, would simplify what you need to assert in order to make sure that the minification happened.

Copy link
Member Author

@matteofigus matteofigus Mar 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the process is referentially transparent , so it should return always the same result

Copy link
Member Author

@matteofigus matteofigus Mar 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ok, so let me update this. We can also change it in the future if this happens to be a problem.

Copy link
Member

Choose a reason for hiding this comment

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

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

expect(minified).to.be.a('string');
expect(minified).not.to.contain('const');
expect(minified).to.contain('var');
expect(minified).not.to.contain('name');
});
});

describe('when file contains not valid js', function(){

var content = 'const a=notvalid(';
var execute = function(){
minifyFile('.js', content);
};

it('should throw an exception', function(){
expect(execute).to.throw('Unexpected token');
});
});
});
});
192 changes: 121 additions & 71 deletions test/unit/cli-domain-package-components.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,96 +6,146 @@ var path = require('path');
var sinon = require('sinon');
var _ = require('underscore');

var initialise = function(){

var fsMock = {
existsSync: sinon.stub(),
lstatSync: sinon.stub(),
mkdirSync: sinon.spy(),
readdirSync: sinon.stub(),
readFileSync: sinon.stub(),
readJson: sinon.stub(),
readJsonSync: sinon.stub(),
writeFile: sinon.stub().yields(null, 'ok'),
writeJson: sinon.stub().yields(null, 'ok')
};

var pathMock = {
extname: path.extname,
join: path.join,
resolve: function(){
return _.toArray(arguments).join('/');
}
};

var Local = injectr('../../src/cli/domain/package-components.js', {
'fs-extra': fsMock,
'uglify-js': {
minify: function(code){
return {
code: code
};
describe('cli : domain : package-components', function(){

var packageStaticFilesStub;

var initialise = function(){

var fsMock = {
existsSync: sinon.stub(),
lstatSync: sinon.stub(),
mkdirSync: sinon.spy(),
readdirSync: sinon.stub(),
readFileSync: sinon.stub(),
readJson: sinon.stub(),
readJsonSync: sinon.stub(),
writeFile: sinon.stub().yields(null, 'ok'),
writeJson: sinon.stub().yields(null, 'ok')
};

var pathMock = {
extname: path.extname,
join: path.join,
resolve: function(){
return _.toArray(arguments).join('/');
}
},
path: pathMock,
'./package-static-files': sinon.stub().yields(null, 'ok'),
'./package-template': sinon.stub().yields(null, { type: 'jade', src: 'template.js', hashKey: '123456'})
}, { __dirname: '' });
};
Copy link
Member

Choose a reason for hiding this comment

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

I see a lot of arrange before running this unit test. maybe this component does too many things?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. But I would separate in a different PR if you don't mind

Copy link
Member

Choose a reason for hiding this comment

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

Sure, happy to pair on this if you like.


var local = new Local();
packageStaticFilesStub = sinon.stub().yields(null, 'ok');

return { local: local, fs: fsMock };
};
var PackageComponents = injectr('../../src/cli/domain/package-components.js', {
'fs-extra': fsMock,
path: pathMock,
'./package-static-files': packageStaticFilesStub,
'./package-template': sinon.stub().yields(null, { type: 'jade', src: 'template.js', hashKey: '123456'})
}, { __dirname: ''});

var executePackaging = function(local, callback){
return local({
componentPath: '.',
minify: false,
verbose: false
}, callback);
};
var packageComponents = new PackageComponents();

describe('cli : domain : local', function(){
return { packageComponents: packageComponents, fs: fsMock };
};

var executePackaging = function(packageComponents, minify, callback){
return packageComponents({
componentPath: '.',
minify: minify,
verbose: false
}, callback);
};

describe('when packaging', function(){

describe('when component is valid', function(){

var component;
beforeEach(function(done){
describe('when minify=true', function(){

var component;
beforeEach(function(done){

var data = initialise();
var data = initialise();

component = {
name: 'helloworld',
oc: {
files: {
template: {
type: 'jade',
src: 'template.jade'
component = {
name: 'helloworld',
oc: {
files: {
static: ['css'],
template: {
type: 'jade',
src: 'template.jade'
}
}
}
},
dependencies: {}
};
},
dependencies: {}
};

data.fs.existsSync.returns(true);
data.fs.readJsonSync.onCall(0).returns(component);
data.fs.readJsonSync.onCall(1).returns({ version: '1.2.3' });
data.fs.existsSync.returns(true);
data.fs.readJsonSync.onCall(0).returns(component);
data.fs.readJsonSync.onCall(1).returns({ version: '1.2.3' });

executePackaging(data.local, done);
});
executePackaging(data.packageComponents, true, done);
});

it('should add version to package.json file', function(){
expect(component.oc.version).to.eql('1.2.3');
});
it('should add version to package.json file', function(){
expect(component.oc.version).to.eql('1.2.3');
});

it('should mark the package.json as a packaged', function(){
expect(component.oc.packaged).to.eql(true);
});

it('should mark the package.json as a packaged', function(){
expect(component.oc.packaged).to.eql(true);
it('should save hash for template in package.json', function(){
expect(component.oc.files.template.hashKey).not.be.empty;
});

it('should minify static resources', function(){
expect(packageStaticFilesStub.args[0][0].minify).to.eql(true);
});
});

it('should save hash for template in package.json', function(){
expect(component.oc.files.template.hashKey).not.be.empty;
describe('when minify=false', function(){

var component;
beforeEach(function(done){

var data = initialise();

component = {
name: 'helloworld',
oc: {
files: {
static: ['css'],
template: {
type: 'jade',
src: 'template.jade'
}
}
},
dependencies: {}
};

data.fs.existsSync.returns(true);
data.fs.readJsonSync.onCall(0).returns(component);
data.fs.readJsonSync.onCall(1).returns({ version: '1.2.3' });

executePackaging(data.packageComponents, false, done);
});

it('should add version to package.json file', function(){
expect(component.oc.version).to.eql('1.2.3');
});

it('should mark the package.json as a packaged', function(){
expect(component.oc.packaged).to.eql(true);
});

it('should save hash for template in package.json', function(){
expect(component.oc.files.template.hashKey).not.be.empty;
});

it('should minify static resources', function(){
expect(packageStaticFilesStub.args[0][0].minify).to.eql(false);
});
});
});
});
Expand Down
Loading