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

Support CommonJS #28

Merged
merged 1 commit into from
Jul 24, 2015
Merged

Support CommonJS #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,27 @@
"enb": ">= 0.8.22"
},
"devDependencies": {
"chai": "3.2.0",
"chai-as-promised": "5.1.0",
"enb": ">= 0.8.22",
"istanbul": "0.3.14",
"jscs": "1.13.1",
"jshint": "2.7.0",
"mocha": "2.2.4",
"mock-enb": "0.0.2",
"mock-fs": "3.0.0",
"must": "0.12.0"
"rimraf": "2.4.2"
},
"scripts": {
"test": "npm run lint && npm run unit",
"lint": "jshint . && jscs -c .jscs.js .",
"unit": "mocha -R spec",
"cover": "istanbul cover _mocha",
"coveralls": "npm i coveralls && npm run cover -- --report lcovonly && cat ./coverage/lcov.info | coveralls"
},
"dependencies": {
"browserify": "10.2.6",
"vow": "0.4.10",
"vow-node": "0.3.0"
}
}
24 changes: 23 additions & 1 deletion techs/node-js.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,16 @@
* nodeConfig.addTech(require('enb-diverse-js/techs/node-js'));
* ```
*/
var EOL = require('os').EOL;
var EOL = require('os').EOL,
browserify = require('browserify'),
Copy link
Member

Choose a reason for hiding this comment

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

@blond, ты говорил, что для забандливания node.js стоит использовать webpack. почему все-таки остановились на browsefiry, он ведь помимо разворачивания депендов еще и пытается добавить код, эмулирующий ноду для браузера?

Copy link

Choose a reason for hiding this comment

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

Присоединяюсь к вопросу @tadatuta

Default shit in Browserify’s bundle.js: 1900 lines
Default shit in Webpack’s bundle.js: 41 lines

Copy link
Member

Choose a reason for hiding this comment

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

помимо разворачивания депендов еще и пытается добавить код, эмулирующий ноду для браузера

@tadatuta, это настраивается.

Вообще Browserify стал получше с тех пор как я на него смотрел для сборки нодового кода. А webpack довольно тяжолый с милионом плагинов даже из коробки, из-за чего сложнее тестировать и немного сложнее поддерживать.

Но мне не сильно принципиально, если все захотят webpack. Просто именно для этой задачи не вижу особой пользы.

Copy link
Member

Choose a reason for hiding this comment

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

Default shit in Browserify’s bundle.js: 1900 lines
Default shit in Webpack’s bundle.js: 41 lines

@tavriaforever, кажется, что это в режиме по умолчанию, когда он все нативные модули превращает в js-код и подклеивает его. Если выставить нужные опции, чтобы нативные модули и модули из node_modules игнорировались, кол-во лишнего кода должно быть примерно одинаковым.

@gela-d, проверь, это пожалуйста. Если browserify действительно генерит лишнего кода сильно больше даже с опциями — нужно использовать webpack.

Copy link
Member

Choose a reason for hiding this comment

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

так а чем в итоге проверка-то закончилась?

Copy link
Member

Choose a reason for hiding this comment

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

@gela-d сколько точно в цифрах?

@tadatuta на глаз было всё ок.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blond а скажите как померить?

Copy link
Member

Choose a reason for hiding this comment

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

Собрать файл, посмотреть на его вес и вычесть вес исходных файлов.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tadatuta @blond разница будет в 200B

promisify = require('vow-node').promisify;

module.exports = require('enb/lib/build-flow').create()
.name('node-js')
.target('target', '?.node.js')
.useFileList(['vanilla.js', 'node.js'])
.defineOption('devMode', true)
.defineOption('bundled', false)
.builder(function (sourceFiles) {
var node = this.node,
dropRequireCacheFunc = [
Expand All @@ -44,6 +48,24 @@ module.exports = require('enb/lib/build-flow').create()
].join(EOL),
devMode = this._devMode || '';

if (this._bundled) {
var files = sourceFiles.map(function (file) {
Copy link
Member

Choose a reason for hiding this comment

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

относительные пути нужны при любом значении bundled, так что их получение стоит вынести перед if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tadatuta почему? Мы же их используем только в контексте этой опции.

UPD: все, поняла, вынесу.
UPD2: как я могу их вынести, если у меня в каждом map используется file.fullname?

Copy link
Member

Choose a reason for hiding this comment

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

и то верно

return node.relativePath(file.fullname);
}),
browserifyOptions = {
basedir: node.getDir(),
builtins: [],
bundleExternal: false
},
renderer = browserify(files, browserifyOptions),
bundle = promisify(renderer.bundle.bind(renderer));

return bundle()
.then(function (buf) {
return buf.toString('utf-8');
});
}

return [
devMode && dropRequireCacheFunc,
sourceFiles.map(function (file) {
Expand Down
1 change: 0 additions & 1 deletion test/mocha.opts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
test/**/*.test.js
--ui bdd
--reporter dot
--require must
10 changes: 7 additions & 3 deletions test/techs/browser-js.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
require('chai')
.use(require('chai-as-promised'))
.should();

var EOL = require('os').EOL,
mock = require('mock-fs'),
FileList = require('enb/lib/file-list'),
Expand Down Expand Up @@ -26,7 +30,7 @@ describe('browser-js', function () {

return build(blocks)
.then(function (content) {
content[0].must.be(reference);
content[0].should.be.equal(reference);
});
});
});
Expand All @@ -45,14 +49,14 @@ describe('browser-js', function () {
it('code must executed in isolation', function () {
return build(blocks, { iife: true }, true)
.then(function () {
globals[0].must.be(2);
globals[0].should.be.equal(2);
});
});

it('code must be executed in the same scoupe', function () {
return build(blocks, null, true)
.then(function () {
globals[0].must.be(1);
globals[0].should.be.equal(1);
});
});
});
Expand Down
171 changes: 147 additions & 24 deletions test/techs/node-js.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
require('chai')
.use(require('chai-as-promised'))
.should();

var fs = require('fs'),
path = require('path'),
mock = require('mock-fs'),
FileList = require('enb/lib/file-list'),
dropRequireCache = require('enb/lib/fs/drop-require-cache'),
MockNode = require('mock-enb/lib/mock-node'),
NodeJsTech = require('../../techs/node-js');
NodeJsTech = require('../../techs/node-js'),
rimraf = require('rimraf');

describe('node-js', function () {
var globals;
Expand All @@ -18,30 +23,43 @@ describe('node-js', function () {
});

describe('join files', function () {
var blocks = {
'block.vanilla.js': 'global.REQUIRED_TECHS.push("vanilla-js");',
'block.node.js': 'global.REQUIRED_TECHS.push("node-js");'
};
var scheme = {
blocks: {
'block.vanilla.js': 'global.REQUIRED_TECHS.push("vanilla-js");',
'block.node.js': 'global.REQUIRED_TECHS.push("node-js");'
}
},
targetPath = path.resolve('./bundle/bundle.node.js');

afterEach(function () {
dropRequireCache(require, targetPath);
});

it('must join all files', function () {
return build(blocks)
return build(scheme)
.then(function () {
globals.must.include('vanilla-js');
globals.must.include('node-js');
require(targetPath);

globals.should.include('vanilla-js');
globals.should.include('node-js');
});
});

it('must join `vanilla.js` file after `node.js` file', function () {
return build(blocks)
return build(scheme)
.then(function () {
globals.indexOf('node-js').must.be.above(globals.indexOf('vanilla-js'));
require(targetPath);

globals.indexOf('node-js').should.be.above(globals.indexOf('vanilla-js'));
});
});
});

describe('devMode', function () {
var blocks = {
'block.node.js': 'global.REQUIRED_TECHS.push("node-js");'
var scheme = {
blocks: {
'block.node.js': 'global.REQUIRED_TECHS.push("node-js");'
}
},
blockPath = path.resolve('./blocks/block.node.js'),
targetPath = path.resolve('./bundle/bundle.node.js');
Expand All @@ -52,37 +70,142 @@ describe('node-js', function () {
});

it('must drop require cache by default', function () {
return build(blocks)
return build(scheme)
.then(function () {
require(targetPath);
fs.writeFileSync(blockPath, 'global.REQUIRED_TECHS.push("fake-node-js");');

dropRequireCache(require, targetPath);
require(targetPath);

globals.must.include('fake-node-js');
globals.should.include('fake-node-js');
});
});

it('must not drop require cache in production mode', function () {
return build(blocks, { devMode: false })
return build(scheme, { devMode: false })
.then(function () {
require(targetPath);
fs.writeFileSync(blockPath, 'global.REQUIRED_TECHS.push("fake-node-js");');
dropRequireCache(require, targetPath);
require(targetPath);

globals.should.include('node-js');
globals.should.have.length(1);
});
});
});

describe('bundled', function () {
var targetPath = path.resolve('./bundle/bundle.node.js');

afterEach(function () {
global.__fake = '';
dropRequireCache(require, targetPath);
});

it('must run code from bundle in isolation of blocks', function () {
var scheme = {
blocks: {
'block.vanilla.js': 'global.REQUIRED_TECHS.push("vanilla-js");',
'block.node.js': 'global.REQUIRED_TECHS.push("node-js");'
}
};
return build(scheme, { bundled: true })
.then(function () {
rimraf.sync('./blocks/');
require(targetPath);
Copy link
Member

Choose a reason for hiding this comment

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

Этот тест оставляет в памяти закэшированный модуль по пути к targetPath. Надо после каждого теста сделать очистку.

Copy link
Member

Choose a reason for hiding this comment

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

Почему require 2 раза?


globals.should.include('node-js');
globals.should.include('vanilla-js');
});
});

it('must use CommonJS module', function () {
var scheme = {
blocks: {
'block.node.js': 'global.__fake = require("fake");'
},
// jscs:disable
node_modules: {
fake: {
'index.js': 'module.exports = "fake";'
}
}
// jscs:enable
};

return build(scheme, { bundled: true })
.then(function () {
dropRequireCache(require, targetPath);
require(targetPath);

global.__fake.should.be.equal('fake');
Copy link
Member

Choose a reason for hiding this comment

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

global.__fake не очищается после выполнения теста.

Очистку можно вынести в afterEach.

})
.then(function () {
dropRequireCache(require, require.resolve('fake'));
})
.fail(function (err) {
dropRequireCache(require, require.resolve('fake'));
throw err;
});
Copy link
Member

Choose a reason for hiding this comment

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

Тут всё же надо подчистить за собой:

.always(function () {
    dropRequireCache(require, 'path/to/fake');
});

});

it('must not browserify base node modules', function () {
global.__type = 'fake';
var scheme = {
blocks: {
'block.node.js': 'global.__type = require("os").type();'
}
};

return build(scheme, { bundled: true })
.then(function () {
dropRequireCache(require, targetPath);
require(targetPath);

globals.must.include('node-js');
globals.must.have.length(1);
global.__type.should.not.equal('fake');
global.__type.should.not.equal('Browser');
Copy link
Member

Choose a reason for hiding this comment

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

global.__fake не очищается после выполнения теста.

});
});

it('must not reveal node_modules', function () {
var scheme = {
blocks: {
'block.node.js': 'require("fake");'
},
// jscs:disable
node_modules: {
fake: {
'index.js': 'module.exports = "fake";'
}
}
// jscs:enable
},
pathToModule = require.resolve('fake');

return build(scheme, { bundled: true })
.then(function () {
dropRequireCache(require, targetPath);
rimraf.sync('./node_modules');

(function () {
require(targetPath);
}).should.throw(/fake/);
})
.then(function () {
dropRequireCache(require, pathToModule);
})
.fail(function (err) {
dropRequireCache(require, pathToModule);
throw err;
});
Copy link
Member

Choose a reason for hiding this comment

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

Тут тоже надо очистить кэш.

});
});
});

function build(blocks, options) {
mock({
blocks: blocks,
bundle: {}
});
function build(scheme, options) {
scheme.bundle = {};
mock(scheme);

var bundle = new MockNode('bundle'),
fileList = new FileList();
Expand All @@ -91,5 +214,5 @@ function build(blocks, options) {

bundle.provideTechData('?.files', fileList);

return bundle.runTechAndRequire(NodeJsTech, options);
return bundle.runTech(NodeJsTech, options);
}