Skip to content

Commit

Permalink
fix #3998: avoid outbase in identifier names
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Dec 20, 2024
1 parent 7236472 commit 0db1b82
Show file tree
Hide file tree
Showing 6 changed files with 194 additions and 25 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@

This can sometimes expose additional minification opportunities.

* Avoid using the parent directory name for determinism ([#3998](https://github.com/evanw/esbuild/issues/3998))

To make generated code more readable, esbuild includes the name of the source file when generating certain variable names within the file. Specifically bundling a CommonJS file generates a variable to store the lazily-evaluated module initializer. However, if a file is named `index.js` (or with a different extension), esbuild will use the name of the parent directory instead for a better name (since many packages have files all named `index.js` but have unique directory names).

This is problematic when the bundle entry point is named `index.js` and the parent directory name is non-deterministic (e.g. a temporary directory created by a build script). To avoid non-determinism in esbuild's output, esbuild will now use `index` instead of the parent directory in this case. Specifically this will happen if the parent directory is equal to esbuild's `outbase` API option, which defaults to the [lowest common ancestor](https://en.wikipedia.org/wiki/Lowest_common_ancestor) of all user-specified entry point paths.

* Experimental support for esbuild on NetBSD ([#3974](https://github.com/evanw/esbuild/pull/3974))

With this release, esbuild now has a published binary executable for [NetBSD](https://www.netbsd.org/) in the [`@esbuild/netbsd-arm64`](https://www.npmjs.com/package/@esbuild/netbsd-arm64) npm package, and esbuild's installer has been modified to attempt to use it when on NetBSD. Hopefully this makes installing esbuild via npm work on NetBSD. This change was contributed by [@bsiegert](https://github.com/bsiegert).
Expand Down
44 changes: 40 additions & 4 deletions internal/bundler/bundler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,29 @@ type tlaCheck struct {
}

func parseFile(args parseArgs) {
pathForIdentifierName := args.keyPath.Text

// Identifier name generation may use the name of the parent folder if the
// file name starts with "index". However, this is problematic when the
// parent folder includes the parent directory of what the developer
// considers to be the root of the source tree. If that happens, strip the
// parent folder to avoid including it in the generated name.
if relative, ok := args.fs.Rel(args.options.AbsOutputBase, pathForIdentifierName); ok {
for {
next := strings.TrimPrefix(strings.TrimPrefix(relative, "../"), "..\\")
if relative == next {
break
}
relative = next
}
pathForIdentifierName = relative
}

source := logger.Source{
Index: args.sourceIndex,
KeyPath: args.keyPath,
PrettyPath: args.prettyPath,
IdentifierName: js_ast.GenerateNonUniqueNameFromPath(args.keyPath.Text),
IdentifierName: js_ast.GenerateNonUniqueNameFromPath(pathForIdentifierName),
}

var loader config.Loader
Expand Down Expand Up @@ -1857,15 +1875,20 @@ func (s *scanner) addEntryPoints(entryPoints []EntryPoint) []graph.EntryPoint {
return nil
}

// Parse all entry points that were resolved successfully
// Determine output paths for all entry points that were resolved successfully
type entryPointToParse struct {
index int
parse func() uint32
}
var entryPointsToParse []entryPointToParse
for i, info := range entryPointInfos {
if info.results == nil {
continue
}

for _, resolveResult := range info.results {
resolveResult := resolveResult
prettyPath := resolver.PrettyPath(s.fs, resolveResult.PathPair.Primary)
sourceIndex := s.maybeParseFile(resolveResult, prettyPath, nil, logger.Range{}, nil, inputKindEntryPoint, nil)
outputPath := entryPoints[i].OutputPath
outputPathWasAutoGenerated := false

Expand Down Expand Up @@ -1900,9 +1923,17 @@ func (s *scanner) addEntryPoints(entryPoints []EntryPoint) []graph.EntryPoint {
outputPathWasAutoGenerated = true
}

// Defer parsing for this entry point until later
entryPointsToParse = append(entryPointsToParse, entryPointToParse{
index: len(entryMetas),
parse: func() uint32 {
return s.maybeParseFile(resolveResult, prettyPath, nil, logger.Range{}, nil, inputKindEntryPoint, nil)
},
})

entryMetas = append(entryMetas, graph.EntryPoint{
OutputPath: outputPath,
SourceIndex: sourceIndex,
SourceIndex: ast.InvalidRef.SourceIndex,
OutputPathWasAutoGenerated: outputPathWasAutoGenerated,
})
}
Expand All @@ -1924,6 +1955,11 @@ func (s *scanner) addEntryPoints(entryPoints []EntryPoint) []graph.EntryPoint {
}
}

// Only parse entry points after "AbsOutputBase" has been determined
for _, toParse := range entryPointsToParse {
entryMetas[toParse.index].SourceIndex = toParse.parse()
}

// Turn all output paths back into relative paths, but this time relative to
// the "outbase" value we computed above
for i := range entryMetas {
Expand Down
48 changes: 48 additions & 0 deletions internal/bundler_tests/bundler_default_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9123,3 +9123,51 @@ func TestStringExportNamesIIFE(t *testing.T) {
},
})
}

func TestSourceIdentifierNameIndexSingleEntry(t *testing.T) {
default_suite.expectBundled(t, bundled{
files: map[string]string{
// Generate identifier names for top-level and nested files
"/Users/user/project/index.js": `
require('.')
require('pkg')
require('./nested')
`,
"/Users/user/project/nested/index.js": `exports.nested = true`,
"/Users/user/project/node_modules/pkg/index.js": `exports.pkg = true`,
},
entryPaths: []string{"/Users/user/project/index.js"},
options: config.Options{
Mode: config.ModeBundle,
AbsOutputDir: "/Users/user/project/out",
},
})
}

func TestSourceIdentifierNameIndexMultipleEntry(t *testing.T) {
default_suite.expectBundled(t, bundled{
files: map[string]string{
// Generate identifier names for top-level and nested files
"/Users/user/project/home/index.js": `
require('.')
require('pkg')
require('../common')
`,
"/Users/user/project/about/index.js": `
require('.')
require('pkg')
require('../common')
`,
"/Users/user/project/common/index.js": `exports.common = true`,
"/Users/user/project/node_modules/pkg/index.js": `exports.pkg = true`,
},
entryPaths: []string{
"/Users/user/project/home/index.js",
"/Users/user/project/about/index.js",
},
options: config.Options{
Mode: config.ModeBundle,
AbsOutputDir: "/Users/user/project/out",
},
})
}
87 changes: 83 additions & 4 deletions internal/bundler_tests/snapshots/snapshots_default.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5946,23 +5946,23 @@ console.log("cache:", require.cache);
TestRequireParentDirCommonJS
---------- /out.js ----------
// Users/user/project/src/index.js
var require_src = __commonJS({
var require_index = __commonJS({
"Users/user/project/src/index.js"(exports, module) {
module.exports = 123;
}
});

// Users/user/project/src/dir/entry.js
console.log(require_src());
console.log(require_index());

================================================================================
TestRequireParentDirES6
---------- /out.js ----------
// Users/user/project/src/index.js
var src_default = 123;
var index_default = 123;

// Users/user/project/src/dir/entry.js
console.log(src_default);
console.log(index_default);

================================================================================
TestRequirePropertyAccessCommonJS
Expand Down Expand Up @@ -6147,6 +6147,85 @@ function fn() {
// entry.js
console.log(fn());

================================================================================
TestSourceIdentifierNameIndexMultipleEntry
---------- /Users/user/project/out/home/index.js ----------
// Users/user/project/node_modules/pkg/index.js
var require_pkg = __commonJS({
"Users/user/project/node_modules/pkg/index.js"(exports) {
exports.pkg = true;
}
});

// Users/user/project/common/index.js
var require_common = __commonJS({
"Users/user/project/common/index.js"(exports) {
exports.common = true;
}
});

// Users/user/project/home/index.js
var require_home = __commonJS({
"Users/user/project/home/index.js"() {
require_home();
require_pkg();
require_common();
}
});
export default require_home();

---------- /Users/user/project/out/about/index.js ----------
// Users/user/project/node_modules/pkg/index.js
var require_pkg = __commonJS({
"Users/user/project/node_modules/pkg/index.js"(exports) {
exports.pkg = true;
}
});

// Users/user/project/common/index.js
var require_common = __commonJS({
"Users/user/project/common/index.js"(exports) {
exports.common = true;
}
});

// Users/user/project/about/index.js
var require_about = __commonJS({
"Users/user/project/about/index.js"() {
require_about();
require_pkg();
require_common();
}
});
export default require_about();

================================================================================
TestSourceIdentifierNameIndexSingleEntry
---------- /Users/user/project/out/index.js ----------
// Users/user/project/node_modules/pkg/index.js
var require_pkg = __commonJS({
"Users/user/project/node_modules/pkg/index.js"(exports) {
exports.pkg = true;
}
});

// Users/user/project/nested/index.js
var require_nested = __commonJS({
"Users/user/project/nested/index.js"(exports) {
exports.nested = true;
}
});

// Users/user/project/index.js
var require_index = __commonJS({
"Users/user/project/index.js"() {
require_index();
require_pkg();
require_nested();
}
});
export default require_index();

================================================================================
TestSourceMap
---------- /Users/user/project/out.js.map ----------
Expand Down
24 changes: 12 additions & 12 deletions internal/bundler_tests/snapshots/snapshots_packagejson.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -683,10 +683,10 @@ TestPackageJsonImportSelfUsingImport
var foo_import_default = "foo";

// Users/user/project/src/index.js
var src_default = "index";
console.log(src_default, foo_import_default);
var index_default = "index";
console.log(index_default, foo_import_default);
export {
src_default as default
index_default as default
};

================================================================================
Expand All @@ -696,10 +696,10 @@ TestPackageJsonImportSelfUsingImportScoped
var foo_import_default = "foo";

// Users/user/project/src/index.js
var src_default = "index";
console.log(src_default, foo_import_default);
var index_default = "index";
console.log(index_default, foo_import_default);
export {
src_default as default
index_default as default
};

================================================================================
Expand All @@ -713,16 +713,16 @@ var require_foo_require = __commonJS({
});

// Users/user/project/src/index.js
var require_src = __commonJS({
var require_index = __commonJS({
"Users/user/project/src/index.js"(exports, module) {
module.exports = "index";
console.log(
require_src(),
require_index(),
require_foo_require()
);
}
});
export default require_src();
export default require_index();

================================================================================
TestPackageJsonImportSelfUsingRequireScoped
Expand All @@ -735,16 +735,16 @@ var require_foo_require = __commonJS({
});

// Users/user/project/src/index.js
var require_src = __commonJS({
var require_index = __commonJS({
"Users/user/project/src/index.js"(exports, module) {
module.exports = "index";
console.log(
require_src(),
require_index(),
require_foo_require()
);
}
});
export default require_src();
export default require_index();

================================================================================
TestPackageJsonImports
Expand Down
10 changes: 5 additions & 5 deletions internal/bundler_tests/snapshots/snapshots_splitting.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,23 @@ var init_a = __esm({
var B;
var init_b = __esm({
"src/b.js"() {
B = async () => (await Promise.resolve().then(() => (init_src(), src_exports))).A;
B = async () => (await Promise.resolve().then(() => (init_index(), index_exports))).A;
}
});

// src/index.js
var src_exports = {};
__export(src_exports, {
var index_exports = {};
__export(index_exports, {
A: () => A,
B: () => B
});
var init_src = __esm({
var init_index = __esm({
"src/index.js"() {
init_a();
init_b();
}
});
init_src();
init_index();
export {
A,
B
Expand Down

0 comments on commit 0db1b82

Please sign in to comment.