lib: no need to strip BOM or shebang for scripts

Backport-PR-URL: #31228 PR-URL: #27375 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs · Jan 8, 2020 · b743f2c · b743f2c
1 parent 6c34ad6
commit b743f2c
Show file tree

Hide file tree

Showing 7 changed files with 21 additions and 66 deletions.
diff --git a/lib/internal/main/check_syntax.js b/lib/internal/main/check_syntax.js
@@ -13,10 +13,6 @@ const {
 
 const { pathToFileURL } = require('url');
 
-const {
-  stripShebangOrBOM,
-} = require('internal/modules/cjs/helpers');
-
 const {
   Module: {
     _resolveFilename: resolveCJSModuleName,
@@ -68,5 +64,5 @@ function checkSyntax(source, filename) {
     }
   }
 
-  wrapSafe(filename, stripShebangOrBOM(source));
+  wrapSafe(filename, source);
 }
diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js
@@ -111,37 +111,6 @@ function stripBOM(content) {
   return content;
 }
 
-/**
- * Find end of shebang line and slice it off
- */
-function stripShebang(content) {
-  // Remove shebang
-  if (content.charAt(0) === '#' && content.charAt(1) === '!') {
-    // Find end of shebang line and slice it off
-    let index = content.indexOf('\n', 2);
-    if (index === -1)
-      return '';
-    if (content.charAt(index - 1) === '\r')
-      index--;
-    // Note that this actually includes the newline character(s) in the
-    // new output. This duplicates the behavior of the regular expression
-    // that was previously used to replace the shebang line.
-    content = content.slice(index);
-  }
-  return content;
-}
-
-// Strip either the shebang or UTF BOM of a file.
-// Note that this only processes one. If both occur in
-// either order, the one that comes second is not
-// significant.
-function stripShebangOrBOM(content) {
-  if (content.charCodeAt(0) === 0xFEFF) {
-    return content.slice(1);
-  }
-  return stripShebang(content);
-}
-
 const builtinLibs = [
   'assert', 'async_hooks', 'buffer', 'child_process', 'cluster', 'crypto',
   'dgram', 'dns', 'domain', 'events', 'fs', 'http', 'http2', 'https', 'net',
@@ -208,6 +177,4 @@ module.exports = {
   makeRequireFunction,
   normalizeReferrerURL,
   stripBOM,
-  stripShebang,
-  stripShebangOrBOM,
 };
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -51,7 +51,7 @@ const {
   makeRequireFunction,
   normalizeReferrerURL,
   stripBOM,
-  stripShebangOrBOM,
+  loadNativeModule
 } = require('internal/modules/cjs/helpers');
 const { getOptionValue } = require('internal/options');
 const enableSourceMaps = getOptionValue('--enable-source-maps');
@@ -961,9 +961,9 @@ function wrapSafe(filename, content) {
     });
   }
 
-  let compiledWrapper;
+  let compiled;
   try {
-    compiledWrapper = compileFunction(
+    compiled = compileFunction(
       content,
       filename,
       0,
@@ -981,36 +981,39 @@ function wrapSafe(filename, content) {
       ]
     );
   } catch (err) {
-    enrichCJSError(err);
+    if (experimentalModules) {
+      enrichCJSError(err);
+    }
     throw err;
   }
 
   if (experimentalModules) {
     const { callbackMap } = internalBinding('module_wrap');
-    callbackMap.set(compiledWrapper, {
+    callbackMap.set(compiled.cacheKey, {
       importModuleDynamically: async (specifier) => {
         const loader = await asyncESM.loaderPromise;
         return loader.import(specifier, normalizeReferrerURL(filename));
       }
     });
   }
 
-  return compiledWrapper;
+  return compiled.function;
 }
 
 // Run the file contents in the correct scope or sandbox. Expose
 // the correct helper variables (require, module, exports) to
 // the file.
 // Returns exception, if any.
 Module.prototype._compile = function(content, filename) {
+  let moduleURL;
+  let redirects;
   if (manifest) {
-    const moduleURL = pathToFileURL(filename);
+    moduleURL = pathToFileURL(filename);
+    redirects = manifest.getRedirector(moduleURL);
     manifest.assertIntegrity(moduleURL, content);
   }
 
-  // Strip after manifest integrity check
-  content = stripShebangOrBOM(content);
-
+  maybeCacheSourceMap(filename, content, this);
   const compiledWrapper = wrapSafe(filename, content);
 
   var inspectorWrapper = null;

diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
@@ -12,7 +12,8 @@ const {
 const { Buffer } = require('buffer');
 
 const {
-  stripBOM
+  stripBOM,
+  loadNativeModule
 } = require('internal/modules/cjs/helpers');
 const CJSModule = require('internal/modules/cjs/loader').Module;
 const internalURLModule = require('internal/url');
@@ -78,9 +79,8 @@ translators.set('module', async function moduleStrategy(url) {
   const source = `${await getSource(url)}`;
   maybeCacheSourceMap(url, source);
   debug(`Translating StandardModule ${url}`);
-  const { ModuleWrap, callbackMap } = internalBinding('module_wrap');
   const module = new ModuleWrap(source, url);
-  callbackMap.set(module, {
+  moduleWrap.callbackMap.set(module, {
     initializeImportMeta,
     importModuleDynamically,
   });

diff --git a/test/fixtures/utf8-bom-shebang-shebang.js b/test/fixtures/utf8-bom-shebang-shebang.js
@@ -0,0 +1,3 @@
+#!shebang
+#!shebang
+module.exports = 42;
diff --git a/test/parallel/test-internal-modules-strip-shebang.js b/test/parallel/test-internal-modules-strip-shebang.js
diff --git a/test/sequential/test-module-loading.js b/test/sequential/test-module-loading.js
@@ -355,7 +355,7 @@ assert.strictEqual(require('../fixtures/utf8-bom.json'), 42);
 // Loading files with BOM + shebang.
 // See https://github.com/nodejs/node/issues/27767
 assert.throws(() => {
-  require('../fixtures/utf8-bom-shebang.js');
+  require('../fixtures/utf8-bom-shebang-shebang.js');
 }, { name: 'SyntaxError' });
 assert.strictEqual(require('../fixtures/utf8-shebang-bom.js'), 42);