From 4d841b3337e4ed7d52273b48eb5317ea01393ae8 Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Sat, 9 Mar 2019 00:07:16 -0800 Subject: [PATCH] esm: --type=auto should throw syntax errors directly --- lib/internal/errors.js | 3 ++- lib/internal/main/check_syntax.js | 3 ++- lib/internal/main/eval_stdin.js | 2 +- lib/internal/main/eval_string.js | 2 +- lib/internal/modules/esm/default_resolve.js | 5 +++-- lib/internal/modules/esm/detect_type.js | 12 ++++++++++-- test/es-module/test-esm-type-auto.js | 10 +++++++++- .../es-modules/type-auto-scope/syntax-error-1.js | 1 + .../es-modules/type-auto-scope/syntax-error-2.js | 2 ++ 9 files changed, 31 insertions(+), 9 deletions(-) create mode 100644 test/fixtures/es-modules/type-auto-scope/syntax-error-1.js create mode 100644 test/fixtures/es-modules/type-auto-scope/syntax-error-2.js diff --git a/lib/internal/errors.js b/lib/internal/errors.js index f98953dede..604cb295de 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -816,7 +816,8 @@ E('ERR_INVALID_SYNC_FORK_INPUT', E('ERR_INVALID_THIS', 'Value of "this" must be of type %s', TypeError); E('ERR_INVALID_TUPLE', '%s must be an iterable %s tuple', TypeError); E('ERR_INVALID_TYPE_FLAG', - 'Type flag must be one of "auto", "commonjs", or "module". Received --type=%s', + 'Type flag must be one of "auto", "commonjs", or "module". ' + + 'Received --type=%s', TypeError); E('ERR_INVALID_URI', 'URI malformed', URIError); E('ERR_INVALID_URL', 'Invalid URL: %s', TypeError); diff --git a/lib/internal/main/check_syntax.js b/lib/internal/main/check_syntax.js index a292b5a774..e9e0b5fb21 100644 --- a/lib/internal/main/check_syntax.js +++ b/lib/internal/main/check_syntax.js @@ -58,7 +58,8 @@ function checkSyntax(source, filename) { if (filename === '[stdin]' || filename === '[eval]') { const { typeFlag } = require('internal/process/esm_loader'); isModule = typeFlag === 'module' || (typeFlag === 'auto' && - require('internal/modules/esm/detect_type')(source) === 'module'); + require('internal/modules/esm/detect_type')(source, filename) === + 'module'); } else { const resolve = require('internal/modules/esm/default_resolve'); const { format } = resolve(pathToFileURL(filename).toString()); diff --git a/lib/internal/main/eval_stdin.js b/lib/internal/main/eval_stdin.js index af6c6e6b2d..2c56c950af 100644 --- a/lib/internal/main/eval_stdin.js +++ b/lib/internal/main/eval_stdin.js @@ -21,7 +21,7 @@ readStdin((code) => { process._eval = code; if (typeFlag === 'module' || (typeFlag === 'auto' && - require('internal/modules/esm/detect_type')(code) === 'module')) + require('internal/modules/esm/detect_type')(code, '[stdin]') === 'module')) evalModule(process._eval); else evalScript('[stdin]', process._eval, process._breakFirstLine); diff --git a/lib/internal/main/eval_string.js b/lib/internal/main/eval_string.js index a45d6589fd..847116b495 100644 --- a/lib/internal/main/eval_string.js +++ b/lib/internal/main/eval_string.js @@ -16,7 +16,7 @@ addBuiltinLibsToObject(global); markBootstrapComplete(); if (typeFlag === 'module' || (typeFlag === 'auto' && - require('internal/modules/esm/detect_type')(source) === 'module')) + require('internal/modules/esm/detect_type')(source, '[eval]') === 'module')) evalModule(source); else evalScript('[eval]', source, process._breakFirstLine); diff --git a/lib/internal/modules/esm/default_resolve.js b/lib/internal/modules/esm/default_resolve.js index b437a072b0..72cad6ba32 100644 --- a/lib/internal/modules/esm/default_resolve.js +++ b/lib/internal/modules/esm/default_resolve.js @@ -102,8 +102,9 @@ function getModuleFormat(url, isMain, parentURL) { // --type=auto detects an ESM .js file within a CommonJS scope. if (isMain && format === 'commonjs' && asyncESM.typeFlag === 'auto') { - const source = readFileSync(fileURLToPath(url), 'utf8'); - format = require('internal/modules/esm/detect_type')(source); + const filename = fileURLToPath(url); + const source = readFileSync(filename, 'utf8'); + format = require('internal/modules/esm/detect_type')(source, filename); } return format; diff --git a/lib/internal/modules/esm/detect_type.js b/lib/internal/modules/esm/detect_type.js index 3d3d44ef95..bbb5f0a8a7 100644 --- a/lib/internal/modules/esm/detect_type.js +++ b/lib/internal/modules/esm/detect_type.js @@ -1,6 +1,9 @@ 'use strict'; const acorn = require('internal/deps/acorn/acorn/dist/acorn'); +const { + stripShebang, stripBOM +} = require('internal/modules/cjs/helpers'); // Detect the module type of a file: CommonJS or ES module. // An ES module, for the purposes of this algorithm, is defined as any @@ -9,7 +12,9 @@ const acorn = require('internal/deps/acorn/acorn/dist/acorn'); // full parse; we can detect import or export statements just from the tokens. // Also as of this writing, Acorn doesn't support import() expressions as they // are only Stage 3; yet Node already supports them. -function detectType(source) { +function detectType(source, filename) { + source = stripShebang(source); + source = stripBOM(source); try { let prevToken, prevPrevToken; for (const { type: token } of acorn.tokenizer(source)) { @@ -30,7 +35,10 @@ function detectType(source) { prevToken = token; } } catch { - return 'commonjs'; + // If the tokenizer threw, there's a syntax error. + // Compile the script, this will throw with an informative error. + const vm = require('vm'); + new vm.Script(source, { displayErrors: true, filename }); } return 'commonjs'; } diff --git a/test/es-module/test-esm-type-auto.js b/test/es-module/test-esm-type-auto.js index 5aae08f6f7..e59fbad86d 100644 --- a/test/es-module/test-esm-type-auto.js +++ b/test/es-module/test-esm-type-auto.js @@ -19,7 +19,10 @@ expect('cjs-with-string-containing-import.js', version); expect('print-version.js', version); expect('ambiguous-with-import-expression.js', version); -function expect(file, want) { +expect('syntax-error-1.js', 'SyntaxError', true); +expect('syntax-error-2.js', 'SyntaxError', true); + +function expect(file, want, wantsError = false) { const argv = [ require.resolve(`../fixtures/es-modules/type-auto-scope/${file}`) ]; @@ -32,6 +35,11 @@ function expect(file, want) { }; exec(process.execPath, argv, opts, common.mustCall((err, stdout, stderr) => { + if (wantsError) { + stdout = stderr; + } else { + assert.ifError(err); + } if (stdout.includes(want)) return; assert.fail( `For ${file}, failed to find ${want} in: <\n${stdout}\n>`); diff --git a/test/fixtures/es-modules/type-auto-scope/syntax-error-1.js b/test/fixtures/es-modules/type-auto-scope/syntax-error-1.js new file mode 100644 index 0000000000..1f5d64097b --- /dev/null +++ b/test/fixtures/es-modules/type-auto-scope/syntax-error-1.js @@ -0,0 +1 @@ +export \ No newline at end of file diff --git a/test/fixtures/es-modules/type-auto-scope/syntax-error-2.js b/test/fixtures/es-modules/type-auto-scope/syntax-error-2.js new file mode 100644 index 0000000000..11d0e77b48 --- /dev/null +++ b/test/fixtures/es-modules/type-auto-scope/syntax-error-2.js @@ -0,0 +1,2 @@ +const str = 'import +var foo = 3; \ No newline at end of file