Skip to content

Commit

Permalink
fix #785: avoid absolute paths for disabled files
Browse files Browse the repository at this point in the history
  • Loading branch information
evanw committed Feb 11, 2021
1 parent 4ab6a6d commit 419b801
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 23 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@

This caused a problem with the synchronous JavaScript API calls which run the esbuild child process in a single-response mode. The ping message was interpreted as a second response and tripped up the message protocol. Pings are only useful for the asynchronous API calls. Running the pings during synchronous API calls was unintentional. With this release pings are no longer run for synchronous API calls so this regression should be fixed.

* Remove absolute paths for disabled files from source maps ([#785](https://github.com/evanw/esbuild/issues/785))

Files can be ignored (i.e. set to empty) using the [`browser` field in `package.json`](https://github.com/defunctzombie/package-browser-field-spec/tree/4f296871cee64e60124841c06c06511885152f19#ignore-a-module). Specifically, you can set the `browser` field to a map where the key is the module name and the value is `false`. This is a convention followed by several bundlers including esbuild.

Previously ignoring a file caused that file's path to appear as an absolute path in any generated source map. This is problematic because it means different source maps will be generated on different systems, since the absolute path contains system-specific directory information. Now esbuild will treat these paths the same way it treats other paths and will put a relative path in the source map.

## 0.8.43

* Support the `XDG_CACHE_HOME` environment variable ([#757](https://github.com/evanw/esbuild/issues/757))
Expand Down
10 changes: 5 additions & 5 deletions internal/bundler/bundler.go
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,11 @@ func runOnLoadPlugins(
}
}

// Force disabled modules to be empty
if source.KeyPath.IsDisabled() {
return loaderPluginResult{loader: config.LoaderJS}, true
}

// Read normal modules from disk
if source.KeyPath.Namespace == "file" {
if contents, err := fsCache.ReadFile(fs, source.KeyPath.Text); err == nil {
Expand All @@ -793,11 +798,6 @@ func runOnLoadPlugins(
}
}

// Force disabled modules to be empty
if source.KeyPath.Namespace == resolver.BrowserFalseNamespace {
return loaderPluginResult{loader: config.LoaderJS}, true
}

// Otherwise, fail to load the path
return loaderPluginResult{loader: config.LoaderNone}, true
}
Expand Down
2 changes: 1 addition & 1 deletion internal/bundler/linker.go
Original file line number Diff line number Diff line change
Expand Up @@ -2224,7 +2224,7 @@ func (c *linkerContext) advanceImportTracker(tracker importTracker) (importTrack

// Is this a disabled file?
otherSourceIndex := *record.SourceIndex
if c.files[otherSourceIndex].source.KeyPath.Namespace == resolver.BrowserFalseNamespace {
if c.files[otherSourceIndex].source.KeyPath.IsDisabled() {
return importTracker{sourceIndex: otherSourceIndex, importRef: js_ast.InvalidRef}, importDisabled, nil
}

Expand Down
6 changes: 3 additions & 3 deletions internal/bundler/snapshots/snapshots_packagejson.txt
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ var index;
================================================================================
TestPackageJsonBrowserMapModuleDisabled
---------- /Users/user/project/out.js ----------
// empty:/Users/user/project/node_modules/node-pkg/index.js
// (disabled):/Users/user/project/node_modules/node-pkg/index.js
var require_node_pkg = __commonJS(() => {
});

Expand Down Expand Up @@ -113,7 +113,7 @@ console.log(import_demo_pkg.default());
================================================================================
TestPackageJsonBrowserMapNativeModuleDisabled
---------- /Users/user/project/out.js ----------
// empty:fs
// (disabled):fs
var require_fs = __commonJS(() => {
});

Expand All @@ -132,7 +132,7 @@ console.log(import_demo_pkg.default());
================================================================================
TestPackageJsonBrowserMapRelativeDisabled
---------- /Users/user/project/out.js ----------
// empty:/Users/user/project/node_modules/demo-pkg/util-node.js
// (disabled):Users/user/project/node_modules/demo-pkg/util-node.js
var require_util_node = __commonJS(() => {
});

Expand Down
16 changes: 15 additions & 1 deletion internal/logger/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,24 @@ func (a SortableMsgs) Less(i int, j int) bool {
type Path struct {
Text string
Namespace string
Flags PathFlags
}

type PathFlags uint8

const (
// This corresponds to a value of "false' in the "browser" package.json field
PathDisabled PathFlags = 1 << iota
)

func (p Path) IsDisabled() bool {
return (p.Flags & PathDisabled) != 0
}

func (a Path) ComesBeforeInSortedOrder(b Path) bool {
return a.Namespace > b.Namespace || (a.Namespace == b.Namespace && a.Text < b.Text)
return a.Namespace > b.Namespace ||
(a.Namespace == b.Namespace && (a.Text < b.Text ||
(a.Text == b.Text && a.Flags < b.Flags)))
}

// This has a custom implementation instead of using "filepath.Dir/Base/Ext"
Expand Down
24 changes: 11 additions & 13 deletions internal/resolver/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@ import (
"github.com/evanw/esbuild/internal/logger"
)

// This namespace is used when a module has been disabled by being mapped to
// "false" using the "browser" field of "package.json".
const BrowserFalseNamespace = "empty"

var defaultMainFields = map[config.Platform][]string{
// Note that this means if a package specifies "main", "module", and
// "browser" then "browser" will win out over "module". This is the
Expand Down Expand Up @@ -399,7 +395,7 @@ func (r *resolver) resolveWithoutSymlinks(sourceDir string, importPath string, k
if packageJSON := importDirInfo.enclosingBrowserScope.packageJSON; packageJSON.browserNonPackageMap != nil {
if remapped, ok := packageJSON.browserNonPackageMap[absPath]; ok {
if remapped == nil {
return &ResolveResult{PathPair: PathPair{Primary: logger.Path{Text: absPath, Namespace: BrowserFalseNamespace}}}
return &ResolveResult{PathPair: PathPair{Primary: logger.Path{Text: absPath, Namespace: "file", Flags: logger.PathDisabled}}}
} else if remappedResult, ok := r.resolveWithoutRemapping(importDirInfo.enclosingBrowserScope, *remapped, kind); ok {
result = remappedResult
checkRelative = false
Expand Down Expand Up @@ -452,13 +448,13 @@ func (r *resolver) resolveWithoutSymlinks(sourceDir string, importPath string, k
if remapped == nil {
// "browser": {"module": false}
if absolute, ok := r.loadNodeModules(importPath, kind, sourceDirInfo); ok {
absolute.Primary = logger.Path{Text: absolute.Primary.Text, Namespace: BrowserFalseNamespace}
absolute.Primary = logger.Path{Text: absolute.Primary.Text, Flags: logger.PathDisabled}
if absolute.HasSecondary() {
absolute.Secondary = logger.Path{Text: absolute.Secondary.Text, Namespace: BrowserFalseNamespace}
absolute.Secondary = logger.Path{Text: absolute.Secondary.Text, Flags: logger.PathDisabled}
}
return &ResolveResult{PathPair: absolute}
} else {
return &ResolveResult{PathPair: PathPair{Primary: logger.Path{Text: importPath, Namespace: BrowserFalseNamespace}}}
return &ResolveResult{PathPair: PathPair{Primary: logger.Path{Text: importPath, Flags: logger.PathDisabled}}}
}
} else {
// "browser": {"module": "./some-file"}
Expand Down Expand Up @@ -489,7 +485,7 @@ func (r *resolver) resolveWithoutSymlinks(sourceDir string, importPath string, k
if packageJSON.browserNonPackageMap != nil {
if remapped, ok := packageJSON.browserNonPackageMap[path.Text]; ok {
if remapped == nil {
path.Namespace = BrowserFalseNamespace
path.Flags |= logger.PathDisabled
} else if remappedResult, ok := r.resolveWithoutRemapping(resultDirInfo.enclosingBrowserScope, *remapped, kind); ok {
*path = remappedResult.Primary
} else {
Expand Down Expand Up @@ -522,11 +518,13 @@ func (r *resolver) PrettyPath(path logger.Path) string {
// These should be platform-independent so our output doesn't depend on which
// operating system it was run. Replace Windows backward slashes with standard
// forward slashes.
return strings.ReplaceAll(path.Text, "\\", "/")
path.Text = strings.ReplaceAll(path.Text, "\\", "/")
} else if path.Namespace != "" {
path.Text = fmt.Sprintf("%s:%s", path.Namespace, path.Text)
}

if path.Namespace != "" {
return fmt.Sprintf("%s:%s", path.Namespace, path.Text)
if path.IsDisabled() {
path.Text = "(disabled):" + path.Text
}

return path.Text
Expand Down Expand Up @@ -1302,7 +1300,7 @@ func (r *resolver) loadNodeModules(path string, kind ast.ImportKind, dirInfo *di
if packageJSON := importDirInfo.enclosingBrowserScope.packageJSON; packageJSON.browserNonPackageMap != nil {
if remapped, ok := packageJSON.browserNonPackageMap[absPath]; ok {
if remapped == nil {
return PathPair{Primary: logger.Path{Text: absPath, Namespace: BrowserFalseNamespace}}, true
return PathPair{Primary: logger.Path{Text: absPath, Namespace: "file", Flags: logger.PathDisabled}}, true
} else if remappedResult, ok := r.resolveWithoutRemapping(importDirInfo.enclosingBrowserScope, *remapped, kind); ok {
return remappedResult, true
}
Expand Down
21 changes: 21 additions & 0 deletions scripts/js-api-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,27 @@ let buildTests = {
assert.strictEqual(json.sourcesContent, void 0)
},

async sourceMapWithDisabledFile({ esbuild, testDir }) {
const input = path.join(testDir, 'in.js')
const disabled = path.join(testDir, 'disabled.js')
const packageJSON = path.join(testDir, 'package.json')
const output = path.join(testDir, 'out.js')
const content = 'exports.foo = require("./disabled")'
await writeFileAsync(input, content)
await writeFileAsync(disabled, 'module.exports = 123')
await writeFileAsync(packageJSON, `{"browser": {"./disabled.js": false}}`)
await esbuild.build({ entryPoints: [input], outfile: output, sourcemap: true, bundle: true })
const result = require(output)
assert.strictEqual(result.foo, void 0)
const resultMap = await readFileAsync(output + '.map', 'utf8')
const json = JSON.parse(resultMap)
assert.strictEqual(json.version, 3)
assert.strictEqual(json.sources[0], path.basename(disabled))
assert.strictEqual(json.sources[1], path.basename(input))
assert.strictEqual(json.sourcesContent[0], '')
assert.strictEqual(json.sourcesContent[1], content)
},

async resolveExtensionOrder({ esbuild, testDir }) {
const input = path.join(testDir, 'in.js');
const inputBare = path.join(testDir, 'module.js')
Expand Down

0 comments on commit 419b801

Please sign in to comment.