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

repl: add top-level static import #17285

Closed
wants to merge 1 commit 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
7 changes: 3 additions & 4 deletions lib/internal/loader/ModuleJob.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,20 +87,19 @@ class ModuleJob {
// ModuleWrap in this module, but all modules in the graph.
dependencyJob.instantiated = resolvedPromise;
}
return this.module;
}

async run() {
const module = await this.instantiate();
await this.instantiate();
try {
module.evaluate();
this.module.evaluate();
} catch (e) {
e.stack;
this.hadError = true;
this.error = e;
throw e;
}
return module;
return this.module;
}
}
Object.setPrototypeOf(ModuleJob.prototype, null);
Expand Down
25 changes: 25 additions & 0 deletions lib/internal/loader/singleton.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';

const Loader = require('internal/loader/Loader');

function newLoader(hooks) {
const loader = new Loader();
Loader.registerImportDynamicallyCallback(loader);
if (hooks)
loader.hook(hooks);

return loader;
}

module.exports = {
loaded: false,
get loader() {
delete this.loader;
this.loader = newLoader();
this.loaded = true;
return this.loader;
},
replace(hooks) {
this.loader = newLoader(hooks);
},
};
79 changes: 79 additions & 0 deletions lib/internal/repl/import.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
'use strict';

const acorn = require('internal/deps/acorn/dist/acorn');
const ESLoader = require('internal/loader/singleton');

/*
Performs importing from static import statments

"a(); import { x, y } from 'some_modules'; x(); y"

would become:

"a(); x(); y"

The symbol is a promise from Promise.all on an array of promises
from Loader#import. When a promise finishes it attaches to
`repl.context` with a getter, so that the imported names behave
like variables not values (live binding).
*/

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this @devsnek 👆

Copy link
Contributor

Choose a reason for hiding this comment

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

🎉🎉🎉

function processTopLevelImport(src, repl) {
let root;
try {
root = acorn.parse(src, { ecmaVersion: 8, sourceType: 'module' });
} catch (err) {
return null;
}

const importLength =
root.body.filter((n) => n.type === 'ImportDeclaration').length;

if (importLength === 0)
return null;

const importPromises = [];
let newBody = '';

for (const index in root.body) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Using for-in to iterate over an array is risky since arrays can have other properties besides numeric indexes. In this case, it looks like you can just use a forEach loop:

root.body.forEach((node, index) => {
  ...
});

Copy link
Member Author

@devsnek devsnek Dec 12, 2017

Choose a reason for hiding this comment

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

i trust acorn to not mess with things like that, but if you think its needed i can make the change

Copy link
Member

@bmeck bmeck Dec 13, 2017

Choose a reason for hiding this comment

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

using for...in, poison to Array.prototype with enumerable properties would also be picked up:

> Array.prototype.foo = 'bar';
'bar'
> for (var k in []) console.log(k)
foo
undefined

const node = root.body[index];
if (node.type === 'ImportDeclaration') {
const lPromise = ESLoader.loader.import(node.source.value)
.then((exports) => {
const { specifiers } = node;
if (specifiers[0].type === 'ImportNamespaceSpecifier') {
Object.defineProperty(repl.context, specifiers[0].local.name, {
enumerable: true,
configurable: true,
get() { return exports; }
});
} else {
const properties = {};
for (const { imported, local } of specifiers) {
const imp = imported ? imported.name : 'default';
properties[local.name] = {
enumerable: true,
configurable: true,
get() { return exports[imp]; }
};
}
Object.defineProperties(repl.context, properties);
}
});

importPromises.push(lPromise);
} else if (node.expression !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this case is unnecessary now. You should be able to evaluate the code with all ImportDeclaration nodes stripped out, once the getters have been registered, without inserting any extra returns or ;s.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any thoughts on this?

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 think its faster to concat a new string than it is to remove chunks, but i could be wrong, if anyone has stats on this please let me know.

const { start, end } = node.expression;
newBody += src.substring(start, end) + ';';
} else {
newBody += src.substring(node.start, node.end);
}
}

return {
promise: Promise.all(importPromises),
body: newBody,
};
}

module.exports = processTopLevelImport;
25 changes: 11 additions & 14 deletions lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,9 @@ const errors = require('internal/errors');
module.exports = Module;

// these are below module.exports for the circular reference
const Loader = require('internal/loader/Loader');
const ModuleJob = require('internal/loader/ModuleJob');
const { createDynamicModule } = require('internal/loader/ModuleWrap');
let ESMLoader;
const ESMLoader = require('internal/loader/singleton');

function stat(filename) {
filename = path.toNamespacedPath(filename);
Expand Down Expand Up @@ -463,17 +462,14 @@ Module._load = function(request, parent, isMain) {
if (isMain && experimentalModules) {
(async () => {
// loader setup
if (!ESMLoader) {
ESMLoader = new Loader();
if (!ESMLoader.loaded) {
const userLoader = process.binding('config').userLoader;
if (userLoader) {
const hooks = await ESMLoader.import(userLoader);
ESMLoader = new Loader();
ESMLoader.hook(hooks);
const hooks = await ESMLoader.loader.import(userLoader);
ESMLoader.replace(hooks);
}
}
Loader.registerImportDynamicallyCallback(ESMLoader);
await ESMLoader.import(getURLFromFilePath(request).pathname);
await ESMLoader.loader.import(getURLFromFilePath(request).pathname);
})()
.catch((e) => {
decorateErrorStack(e);
Expand Down Expand Up @@ -577,22 +573,23 @@ Module.prototype.load = function(filename) {
Module._extensions[extension](this, filename);
this.loaded = true;

if (ESMLoader) {
if (ESMLoader.loaded) {
const { loader } = ESMLoader;
const url = getURLFromFilePath(filename);
const urlString = `${url}`;
const exports = this.exports;
if (ESMLoader.moduleMap.has(urlString) !== true) {
ESMLoader.moduleMap.set(
if (loader.moduleMap.has(urlString) !== true) {
loader.moduleMap.set(
urlString,
new ModuleJob(ESMLoader, url, async () => {
new ModuleJob(loader, url, async () => {
const ctx = createDynamicModule(
['default'], url);
ctx.reflect.exports.default.set(exports);
return ctx;
})
);
} else {
const job = ESMLoader.moduleMap.get(urlString);
const job = loader.moduleMap.get(urlString);
if (job.reflect)
job.reflect.exports.default.set(exports);
}
Expand Down
13 changes: 13 additions & 0 deletions lib/repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@

const internalModule = require('internal/module');
const { processTopLevelAwait } = require('internal/repl/await');
const processTopLevelImport = require('internal/repl/import');
const internalUtil = require('internal/util');
const { isTypedArray } = require('internal/util/types');
const util = require('util');
Expand All @@ -60,6 +61,8 @@ const domain = require('domain');
const debug = util.debuglog('repl');
const errors = require('internal/errors');

const experimentalModules = !!process.binding('config').experimentalModules;

const parentModule = module;
const replMap = new WeakMap();

Expand Down Expand Up @@ -222,6 +225,16 @@ function REPLServer(prompt,
}
}

if (!wrappedCmd && experimentalModules && /\bimport\b/.test(code)) {
const processed = processTopLevelImport(code, self);
if (processed !== null) {
processed.promise.then(() => {
defaultEval(processed.body, context, file, cb);
}, (e) => { cb(e); });
return;
}
}

// first, create the Script object to check the syntax

if (code === '\n')
Expand Down
2 changes: 2 additions & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
'lib/internal/loader/ModuleWrap.js',
'lib/internal/loader/ModuleRequest.js',
'lib/internal/loader/search.js',
'lib/internal/loader/singleton.js',
'lib/internal/safe_globals.js',
'lib/internal/net.js',
'lib/internal/module.js',
Expand All @@ -121,6 +122,7 @@
'lib/internal/readline.js',
'lib/internal/repl.js',
'lib/internal/repl/await.js',
'lib/internal/repl/import.js',
'lib/internal/socket_list.js',
'lib/internal/test/unicode.js',
'lib/internal/tls.js',
Expand Down
9 changes: 9 additions & 0 deletions test/fixtures/esm-with-basic-exports.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export default 'default';

export const six = 6;

export let i = 0;
export const increment = () => { i++; };

const _delete = 'delete';
export { _delete as delete };
145 changes: 145 additions & 0 deletions test/parallel/test-repl-top-level-import.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

nit, we need to add tests for the following cases so they do not change over time #17285 (comment) .


const common = require('../common');
const assert = require('assert');
const { stripVTControlCharacters } = require('internal/readline');
const repl = require('repl');

common.crashOnUnhandledRejection();

// Flags: --expose-internals --experimental-modules

const PROMPT = 'import repl > ';

class REPLStream extends common.ArrayStream {
constructor() {
super();
this.waitingForResponse = false;
this.lines = [''];
}

write(chunk, encoding, callback) {
if (Buffer.isBuffer(chunk)) {
chunk = chunk.toString(encoding);
}
const chunkLines = stripVTControlCharacters(chunk).split('\n');
this.lines[this.lines.length - 1] += chunkLines[0];
if (chunkLines.length > 1) {
this.lines.push(...chunkLines.slice(1));
}
this.emit('line');
if (callback) callback();
return true;
}

wait(lookFor = PROMPT) {
if (this.waitingForResponse) {
throw new Error('Currently waiting for response to another command');
}
this.lines = [''];
return common.fires(new Promise((resolve, reject) => {
const onError = (err) => {
this.removeListener('line', onLine);
reject(err);
};
const onLine = () => {
if (this.lines[this.lines.length - 1].includes(lookFor)) {
this.removeListener('error', onError);
this.removeListener('line', onLine);
resolve(this.lines);
}
};
this.once('error', onError);
this.on('line', onLine);
}), new Error(), 1000);
}
}

const putIn = new REPLStream();
const testMe = repl.start({
prompt: PROMPT,
stream: putIn,
terminal: true,
useColors: false,
breakEvalOnSigint: true
});

function runAndWait(cmds, lookFor) {
const promise = putIn.wait(lookFor);
for (const cmd of cmds) {
if (typeof cmd === 'string') {
putIn.run([cmd]);
} else {
testMe.write('', cmd);
}
}
return promise;
}

async function runEach(cmds) {
const out = [];
for (const cmd of cmds) {
const ret = await runAndWait([cmd]);
out.push(...ret);
}
return out;
}

const file = './test/fixtures/esm-with-basic-exports.mjs';
async function main() {
assert.deepStrictEqual(await runEach([
`/* comment */ import { six } from "${file}"; six + 1;`
]), [
`/* comment */ import { six } from "${file}"; six + 1;\r`,
'7',
PROMPT
]);

assert.deepStrictEqual(await runEach([
`import def from "${file}"`,
'def',
]), [
`import def from "${file}"\r`,
'undefined',
PROMPT,
'def\r',
'\'default\'',
PROMPT
]);

testMe.resetContext();

assert.deepStrictEqual(await runEach([
`import * as test from "${file}"`,
'test.six',
]), [
`import * as test from "${file}"\r`,
'undefined',
PROMPT,
'test.six\r',
'6',
PROMPT
]);

assert.deepStrictEqual(await runEach([
`import { i, increment } from "${file}"`,
'i',
'increment()',
'i',
]), [
`import { i, increment } from "${file}"\r`,
'undefined',
PROMPT,
'i\r',
'0',
PROMPT,
'increment()\r',
'undefined',
PROMPT,
'i\r',
'1',
PROMPT,
]);
}

main();