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.

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

src: add require transform pipeline #12349

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
49 changes: 49 additions & 0 deletions doc/api/globals.md
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,55 @@ added: v0.3.0
Use the internal `require()` machinery to look up the location of a module,
but rather than loading the module, just return the resolved filename.

### require.addTransform(transform)
Copy link
Contributor

Choose a reason for hiding this comment

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

Type of transform and the return type is missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The entire .md file appears to be lacking argument and return type info, actually...

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the transform type. It doesn't have a return value though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work on builtin modules?


* `transform` {Function} A function to use to transform module text given the
content and full path to the file.

Add a transform function to modify contents of a file loaded with `require()`
before passing it to the parser. This enables pre-compile transformations such
as transpiling code, replacing an interface with mock functions for testing or
instrumenting code for purposes such as gathering code coverage metrics or
tracing application behavior and measuring performance.

The `transform` function accepts the content string and full file path as the
first and second arguments. The function is expected to return a new content
string as the result of the transformation.

Multiple transforms may be added to the pipeline and will be applied in the
order they are added.


```js
require.addTransform((content, filepath) => {
Copy link

Choose a reason for hiding this comment

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

filename?

// Only apply the transform to a specific file
if (!/my\-module\.js$/.test(filename)) {
return content;
}
return `
${content}
const originalFunction = exports.someFunction;
exports.someFunction = function myMockedFunction(first, second, callback) {
var start = process.hrtime();
function wrappedCallback() {
var end = process.hrtime(start);
console.log('execution time: %ds', end[0] + (end[1] / 1000000000));
return callback(this, arguments);
}
return someFunction.call(null, first, second, wrappedCallback);
}
`;
});
```

### require.removeTransform(transform)

* `transform` {Function} A function previously given to `require.addTransform()`
which will be removed from the transform pipeline.

Removes a transform function from the transform pipeline so it will no longer
be applied to the contents of files loaded in future `require()` calls.

## setImmediate(callback[, ...args])
<!-- YAML
added: v0.9.1
Expand Down
10 changes: 8 additions & 2 deletions lib/internal/bootstrap_node.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

(function(process) {

let internalModule;

function startup() {
const EventEmitter = NativeModule.require('events');
process._eventsCount = 0;
Expand All @@ -34,6 +36,7 @@
}

const _process = NativeModule.require('internal/process');
internalModule = NativeModule.require('internal/module');

_process.setup_hrtime();
_process.setup_cpuUsage();
Expand Down Expand Up @@ -118,7 +121,6 @@
// '--interactive'
preloadModules();

const internalModule = NativeModule.require('internal/module');
internalModule.addBuiltinLibsToObject(global);
run(() => {
evalScript('[eval]');
Expand Down Expand Up @@ -445,7 +447,6 @@
function checkScriptSyntax(source, filename) {
const Module = NativeModule.require('module');
const vm = NativeModule.require('vm');
const internalModule = NativeModule.require('internal/module');

// remove shebang and BOM
source = internalModule.stripBOM(source.replace(/^#!.*/, ''));
Expand Down Expand Up @@ -544,6 +545,11 @@

NativeModule.prototype.compile = function() {
var source = NativeModule.getSource(this.id);
// NOTE: internalModule is loaded through NativeModule,
// so this step needs to be skipped until after that.
if (internalModule) {
source = internalModule.applyTransforms(source, this.filename);
}
source = NativeModule.wrap(source);

this.loading = true;
Expand Down
30 changes: 29 additions & 1 deletion lib/internal/module.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,39 @@
'use strict';

const assert = require('assert');

exports = module.exports = {
makeRequireFunction,
stripBOM,
addBuiltinLibsToObject
addBuiltinLibsToObject,
applyTransforms
};

exports.requireDepth = 0;

const transforms = [];

function addTransform(transform) {
assert(typeof transform === 'function');
transforms.push(transform);
}
function removeTransform(transform) {
const index = transforms.indexOf(transform);
if (index >= 0) {
transforms.splice(index, 1);
}
}

function applyTransforms(content, filename) {
return transforms.reduce((content, transform) => {
const next = transform(content, filename);
if (typeof next !== 'string') {
throw new Error('Module transforms must return the modified content');
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to make it known which module transform is faulty?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could always attach the failing function to the error object before throwing.

}
return next;
}, content);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

So you reimplemented EventEmmiter only with an explicit return. Do you really feel it's worth the duplication?
You could instead just wrap it, and win!

var ee = new EventEmitter();
var tMap = new WeakMap();
function applyTransforms(content, filename) {
  var e = {content, filename};
  ee.emit('moduleLoading', e);
  return e.content;
}
function addTransform(transform) {
  var t = (e) => {e.content = transform(e.content, e.filename)};
  tmap.add(transform, t);
  ee.addListener('moduleLoading', t)
}
function removeTransform(transform) { ee.removeListener('moduleLoading', tMap.get(transform))}

Copy link
Member Author

@Qard Qard Apr 14, 2017

Choose a reason for hiding this comment

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

Event emitters are much bigger and more complicated than this needs.

That event emitter implementation has a lot more closures and object allocations. Event emitters also inherently have a much more complicated lookup scheme.

As require is a major startup hot-path, it's best not to put event emitters in the middle of that. Keep in mind this code would typically run many thousands of times during app startup.

Copy link
Contributor

Choose a reason for hiding this comment

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

premature optimization is the root of all evil...
IMHO code duplication is worse than a minor performance impact. I'm sure the actual compile is orders of magnitude heavier.
Also EE are super fast when there are no listeners.
Let's test and see...

Copy link
Member Author

@Qard Qard Apr 14, 2017

Choose a reason for hiding this comment

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

Certainly code duplication is a thing to avoid generally, but the code to wrap an event emitter around it has just as much surface area as the custom implementation itself, without even taking into account all the extra stuff running inside the event emitter implementation. It's also a lot more difficult for someone unfamiliar with the code to understand at first glance. Were this something more complicated, an event emitter implementation would definitely make more sense, but it has little advantage here.

Also, the event emitter implementation is 2.5-3 times slower and has 11 times the memory footprint. It's not premature optimization, it's carefully considered and diligently measured optimization. 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Empirical measurement wins (even named my first company Empeeric)!

// Invoke with makeRequireFunction(module) where |module| is the Module object
// to use as the context for the require() function.
function makeRequireFunction(mod) {
Expand All @@ -27,6 +53,8 @@ function makeRequireFunction(mod) {
}

require.resolve = resolve;
require.addTransform = addTransform;
require.removeTransform = removeTransform;

require.main = process.mainModule;

Expand Down
2 changes: 2 additions & 0 deletions lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,8 @@ var resolvedArgv;
// the file.
// Returns exception, if any.
Module.prototype._compile = function(content, filename) {
content = internalModule.applyTransforms(content, filename);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what the performance impact is here.


// Remove shebang
var contLen = content.length;
if (contLen >= 2) {
Expand Down
38 changes: 38 additions & 0 deletions test/parallel/test-require-transforms.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';

const common = require('../common');
const assert = require('assert');

// Juts so lint doesn't complain about unused common (which is required)
common.refreshTmpDir();

function fooTransform(content, filename) {
if (!/test-require-transforms\.js$/.test(filename)) {
return content;
}
return 'module.exports = "foo"';
}
function barTransform(content, filename) {
if (!/test-require-transforms\.js$/.test(filename)) {
return content;
}
return content.replace(/foo/, 'bar');
}

// Test one-step transform
require.addTransform(fooTransform);
delete require.cache[require.resolve('./test-require-transforms')];
const oneStep = require('./test-require-transforms');
assert.strictEqual(oneStep, 'foo');

// Test two-step transform
require.addTransform(barTransform);
delete require.cache[require.resolve('./test-require-transforms')];
const twoStep = require('./test-require-transforms');
assert.strictEqual(twoStep, 'bar');

// Test transform step removal
require.removeTransform(barTransform);
delete require.cache[require.resolve('./test-require-transforms')];
const removal = require('./test-require-transforms');
assert.strictEqual(removal, 'foo');