Skip to content

Commit

Permalink
fix #1760: strip "node:" prefix to fix broken code
Browse files Browse the repository at this point in the history
  • Loading branch information
evanw committed Nov 20, 2021
1 parent 93ed5dd commit 6b4f970
Show file tree
Hide file tree
Showing 6 changed files with 140 additions and 3 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@

However, some people have packages just for TypeScript type definitions. These package can actually have a side effect as they can augment the type of the global object in TypeScript, even if they are marked with `"sideEffects": false`. To avoid warning in this case, esbuild will now only issue this warning if the imported file is non-empty. If the file is empty, then it's irrelevant whether you import it or not so any import of that file does not indicate a bug. This fixes this case because `.d.ts` files typically end up being empty after esbuild parses them since they typically only contain type declarations.

* Attempt to fix packages broken due to the `node:` prefix ([#1760](https://github.com/evanw/esbuild/issues/1760))

Some people have started using the node-specific `node:` path prefix in their packages. This prefix forces the following path to be interpreted as a node built-in module instead of a package on the file system. So `require("node:path")` will always import [node's `path` module](https://nodejs.org/api/path.html) and never import [npm's `path` package](https://www.npmjs.com/package/path).

Adding the `node:` prefix breaks that code with older node versions that don't understand the `node:` prefix. This is a problem with the package, not with esbuild. The package should be adding a fallback if the `node:` prefix isn't available. However, people still want to be able to use these packages with older node versions even though the code is broken. Now esbuild will automatically strip this prefix if it detects that the code will break in the configured target environment (as specified by `--target=`). Note that this only happens during bundling, since import paths are only examined during bundling.

## 0.13.14

* Fix dynamic `import()` on node 12.20+ ([#1772](https://github.com/evanw/esbuild/issues/1772))
Expand Down
5 changes: 4 additions & 1 deletion internal/bundler/bundler.go
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,10 @@ func parseFile(args parseArgs) {
// All "require.resolve()" imports should be external because we don't
// want to waste effort traversing into them
if record.Kind == ast.ImportRequireResolve {
if !record.HandlesImportErrors && (resolveResult == nil || !resolveResult.IsExternal) {
if resolveResult != nil && resolveResult.IsExternal {
// Allow path substitution as long as the result is external
result.resolveResults[importRecordIndex] = resolveResult
} else if !record.HandlesImportErrors {
args.log.AddRangeWarning(&tracker, record.Range,
fmt.Sprintf("%q should be marked as external for use with \"require.resolve\"", record.Path.Text))
}
Expand Down
8 changes: 8 additions & 0 deletions internal/compat/js_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ const (
LogicalAssignment
NestedRestBinding
NewTarget
NodeColonPrefixImport
NodeColonPrefixRequire
NullishCoalescing
ObjectAccessors
ObjectExtensions
Expand Down Expand Up @@ -357,6 +359,12 @@ var jsTable = map[JSFeature]map[Engine][]versionRange{
Node: {{start: v{5, 0, 0}}},
Safari: {{start: v{10, 0, 0}}},
},
NodeColonPrefixImport: {
Node: {{start: v{12, 20, 0}, end: v{13, 0, 0}}, {start: v{14, 13, 1}}},
},
NodeColonPrefixRequire: {
Node: {{start: v{14, 18, 0}, end: v{15, 0, 0}}, {start: v{16, 0, 0}}},
},
NullishCoalescing: {
Chrome: {{start: v{80, 0, 0}}},
Edge: {{start: v{80, 0, 0}}},
Expand Down
44 changes: 42 additions & 2 deletions internal/resolver/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/evanw/esbuild/internal/ast"
"github.com/evanw/esbuild/internal/cache"
"github.com/evanw/esbuild/internal/compat"
"github.com/evanw/esbuild/internal/config"
"github.com/evanw/esbuild/internal/fs"
"github.com/evanw/esbuild/internal/helpers"
Expand Down Expand Up @@ -288,8 +289,7 @@ func (rr *resolver) Resolve(sourceDir string, importPath string, kind ast.Import
strings.HasPrefix(importPath, "//") ||

// "import fs from 'fs'"
// "import fs from 'node:fs'"
(r.options.Platform == config.PlatformNode && (BuiltInNodeModules[importPath] || strings.HasPrefix(importPath, "node:"))) {
(r.options.Platform == config.PlatformNode && BuiltInNodeModules[importPath]) {

if r.debugLogs != nil {
r.debugLogs.addNote("Marking this path as implicitly external")
Expand All @@ -302,6 +302,46 @@ func (rr *resolver) Resolve(sourceDir string, importPath string, kind ast.Import
}, debugMeta
}

// "import fs from 'node:fs'"
// "require('node:fs')"
if r.options.Platform == config.PlatformNode && strings.HasPrefix(importPath, "node:") {
if r.debugLogs != nil {
r.debugLogs.addNote("Marking this path as implicitly external due to the \"node:\" prefix")
}

// Check whether the path will end up as "import" or "require"
convertImportToRequire := !r.options.OutputFormat.KeepES6ImportExportSyntax()
isImport := !convertImportToRequire && (kind == ast.ImportStmt || kind == ast.ImportDynamic)
isRequire := kind == ast.ImportRequire || kind == ast.ImportRequireResolve ||
(convertImportToRequire && (kind == ast.ImportStmt || kind == ast.ImportDynamic))

// Check for support with "import"
if isImport && r.options.UnsupportedJSFeatures.Has(compat.NodeColonPrefixImport) {
if r.debugLogs != nil {
r.debugLogs.addNote("Removing the \"node:\" prefix because the target environment doesn't support it with \"import\" statements")
}

// Automatically strip the prefix if it's not supported
importPath = importPath[5:]
}

// Check for support with "require"
if isRequire && r.options.UnsupportedJSFeatures.Has(compat.NodeColonPrefixRequire) {
if r.debugLogs != nil {
r.debugLogs.addNote("Removing the \"node:\" prefix because the target environment doesn't support it with \"require\" calls")
}

// Automatically strip the prefix if it's not supported
importPath = importPath[5:]
}

r.flushDebugLogs(flushDueToSuccess)
return &ResolveResult{
PathPair: PathPair{Primary: logger.Path{Text: importPath}},
IsExternal: true,
}, debugMeta
}

if parsed, ok := ParseDataURL(importPath); ok {
// "import 'data:text/javascript,console.log(123)';"
// "@import 'data:text/css,body{background:white}';"
Expand Down
14 changes: 14 additions & 0 deletions scripts/compat-table.js
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,20 @@ versions.DynamicImport.node = [
{ start: [13, 2] },
]

// Manually copied from https://nodejs.org/api/esm.html#node-imports
versions.NodeColonPrefixImport = {
node: [
{ start: [12, 20], end: [13] },
{ start: [14, 13, 1] },
]
}
versions.NodeColonPrefixRequire = {
node: [
{ start: [14, 18], end: [15] },
{ start: [16] },
]
}

mergeVersions('ArbitraryModuleNamespaceNames', {
// From https://github.com/tc39/ecma262/pull/2154#issuecomment-825201030
chrome90: true,
Expand Down
66 changes: 66 additions & 0 deletions scripts/js-api-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -2410,6 +2410,72 @@ require("/assets/file.png");
assert.strictEqual(outputFiles[0].path, path.join(testDir, 'entry', 'out', 'TIORPBNU-1.js'))
assert.strictEqual(outputFiles[1].path, path.join(testDir, 'entry', 'out', '3KY7NOSR-2.js'))
},

async nodeColonPrefixImport({ esbuild }) {
const tryTargetESM = async target => {
const result = await esbuild.build({
stdin: { contents: `import 'node:fs'; import('node:fs')` },
bundle: true,
platform: 'node',
target,
format: 'esm',
write: false,
})
const code = result.outputFiles[0].text
return code.slice(code.indexOf(`// <stdin>\n`))
}

assert.strictEqual(await tryTargetESM('node14.13.1'), `// <stdin>\nimport "node:fs";\nimport("node:fs");\n`)
assert.strictEqual(await tryTargetESM('node14.13.0'), `// <stdin>\nimport "fs";\nimport("fs");\n`)
assert.strictEqual(await tryTargetESM('node13'), `// <stdin>\nimport "fs";\nPromise.resolve().then(() => __toModule(__require("fs")));\n`)
assert.strictEqual(await tryTargetESM('node12.99'), `// <stdin>\nimport "node:fs";\nimport("node:fs");\n`)
assert.strictEqual(await tryTargetESM('node12.20'), `// <stdin>\nimport "node:fs";\nimport("node:fs");\n`)
assert.strictEqual(await tryTargetESM('node12.19'), `// <stdin>\nimport "fs";\nPromise.resolve().then(() => __toModule(__require("fs")));\n`)
},

async nodeColonPrefixRequire({ esbuild }) {
const tryTargetESM = async target => {
const result = await esbuild.build({
stdin: { contents: `require('node:fs'); require.resolve('node:fs')` },
bundle: true,
platform: 'node',
target,
format: 'cjs',
write: false,
})
const code = result.outputFiles[0].text
return code.slice(code.indexOf(`// <stdin>\n`))
}

assert.strictEqual(await tryTargetESM('node16'), `// <stdin>\nrequire("node:fs");\nrequire.resolve("node:fs");\n`)
assert.strictEqual(await tryTargetESM('node15.99'), `// <stdin>\nrequire("fs");\nrequire.resolve("fs");\n`)
assert.strictEqual(await tryTargetESM('node15'), `// <stdin>\nrequire("fs");\nrequire.resolve("fs");\n`)
assert.strictEqual(await tryTargetESM('node14.99'), `// <stdin>\nrequire("node:fs");\nrequire.resolve("node:fs");\n`)
assert.strictEqual(await tryTargetESM('node14.18'), `// <stdin>\nrequire("node:fs");\nrequire.resolve("node:fs");\n`)
assert.strictEqual(await tryTargetESM('node14.17'), `// <stdin>\nrequire("fs");\nrequire.resolve("fs");\n`)
},

async nodeColonPrefixImportTurnedIntoRequire({ esbuild }) {
const tryTargetESM = async target => {
const result = await esbuild.build({
stdin: { contents: `import 'node:fs'; import('node:fs')` },
bundle: true,
platform: 'node',
target,
format: 'cjs',
write: false,
})
const code = result.outputFiles[0].text
return code.slice(code.indexOf(`// <stdin>\n`))
}

assert.strictEqual(await tryTargetESM('node16'), `// <stdin>\nvar import_node_fs = __toModule(require("node:fs"));\nimport("node:fs");\n`)
assert.strictEqual(await tryTargetESM('node15.99'), `// <stdin>\nvar import_node_fs = __toModule(require("fs"));\nimport("fs");\n`)
assert.strictEqual(await tryTargetESM('node15'), `// <stdin>\nvar import_node_fs = __toModule(require("fs"));\nimport("fs");\n`)
assert.strictEqual(await tryTargetESM('node14.99'), `// <stdin>\nvar import_node_fs = __toModule(require("node:fs"));\nimport("node:fs");\n`)
assert.strictEqual(await tryTargetESM('node14.18'), `// <stdin>\nvar import_node_fs = __toModule(require("node:fs"));\nimport("node:fs");\n`)
assert.strictEqual(await tryTargetESM('node14.17'), `// <stdin>\nvar import_node_fs = __toModule(require("fs"));\nimport("fs");\n`)
},
}

function fetch(host, port, path, headers) {
Expand Down

0 comments on commit 6b4f970

Please sign in to comment.