From c7be8d8a6b7de838b0656661e04a4b5d376eeb30 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Mon, 28 Jun 2021 11:35:55 -0700 Subject: [PATCH] fix #1044: correct relative paths for file loader --- CHANGELOG.md | 59 ++++ internal/bundler/bundler.go | 92 +++--- internal/bundler/bundler_loader_test.go | 267 ++++++++++++++++++ internal/bundler/linker.go | 24 +- .../bundler/snapshots/snapshots_loader.txt | 122 ++++++++ internal/graph/input.go | 1 + scripts/js-api-tests.js | 2 +- 7 files changed, 525 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b7f86cd2bb..043142c7a56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,64 @@ # Changelog +## Unreleased + +* Fix `file` loader import paths when subdirectories are present ([#1044](https://github.com/evanw/esbuild/issues/1044)) + + Using the `file` loader for a file type causes importing affected files to copy the file into the output directory and to embed the path to the copied file into the code that imported it. However, esbuild previously always embedded the path relative to the output directory itself. This is problematic when the importing code is generated within a subdirectory inside the output directory, since then the relative path is wrong. For example: + + ``` + $ cat src/example/entry.css + div { background: url(../images/image.png) } + + $ esbuild --bundle src/example/entry.css --outdir=out --outbase=src --loader:.png=file + + $ find out -type f + out/example/entry.css + out/image-55DNWN2R.png + + $ cat out/example/entry.css + /* src/example/entry.css */ + div { + background: url(./image-55DNWN2R.png); + } + ``` + + This is output from the previous version of esbuild. The above asset reference in `out/example/entry.css` is wrong. The path should start with `../` because the two files are in different directories. + + With this release, the asset references present in output files will now be the full relative path from the output file to the asset, so imports should now work correctly when the entry point is in a subdirectory within the output directory. This change affects asset reference paths in both CSS and JS output files. + + Note that if you want asset reference paths to be independent of the subdirectory in which they reside, you can use the `--public-path` setting to provide the common path that all asset reference paths should be constructed relative to. + +* Add support for `[dir]` in `--asset-names` ([#1196](https://github.com/evanw/esbuild/pull/1196)) + + You can now use path templates such as `--asset-names=[dir]/[name]-[hash]` to copy the input directory structure of your asset files (i.e. input files loaded with the `file` loader) to the output directory. Here's an example: + + ``` + $ cat entry.css + header { + background: url(images/common/header.png); + } + main { + background: url(images/home/hero.png); + } + + $ esbuild --bundle entry.css --outdir=out --asset-names=[dir]/[name]-[hash] --loader:.png=file + + $ find out -type f + out/images/home/hero-55DNWN2R.png + out/images/common/header-55DNWN2R.png + out/entry.css + + $ cat out/entry.css + /* entry.css */ + header { + background: url(./images/common/header-55DNWN2R.png); + } + main { + background: url(./images/home/hero-55DNWN2R.png); + } + ``` + ## 0.12.11 * Enable faster synchronous transforms with the JS API by default ([#1000](https://github.com/evanw/esbuild/issues/1000)) diff --git a/internal/bundler/bundler.go b/internal/bundler/bundler.go index a18b472221d..7df3c4eff3b 100644 --- a/internal/bundler/bundler.go +++ b/internal/bundler/bundler.go @@ -87,6 +87,7 @@ type parseArgs struct { results chan parseResult inject chan config.InjectedFile skipResolve bool + uniqueKeyPrefix string } type parseResult struct { @@ -274,28 +275,11 @@ func parseFile(args parseArgs) { result.ok = true case config.LoaderFile: - // Add a hash to the file name to prevent multiple files with the same name - // but different contents from colliding - var hash string - if config.HasPlaceholder(args.options.AssetPathTemplate, config.HashPlaceholder) { - h := xxhash.New() - h.Write([]byte(source.Contents)) - hash = hashForFileName(h.Sum(nil)) - } - dir := "/" - relPath := config.TemplateToString(config.SubstituteTemplate(args.options.AssetPathTemplate, config.PathPlaceholders{ - Dir: &dir, - Name: &base, - Hash: &hash, - })) + ext - - // Determine the final path that this asset will have in the output directory - publicPath := joinWithPublicPath(args.options.PublicPath, relPath+source.KeyPath.IgnoredSuffix) - - // Export the resulting relative path as a string - expr := js_ast.Expr{Data: &js_ast.EString{Value: js_lexer.StringToUTF16(publicPath)}} + uniqueKey := fmt.Sprintf("%sA%08d", args.uniqueKeyPrefix, args.sourceIndex) + uniqueKeyPath := uniqueKey + source.KeyPath.IgnoredSuffix + expr := js_ast.Expr{Data: &js_ast.EString{Value: js_lexer.StringToUTF16(uniqueKeyPath)}} ast := js_parser.LazyExportAST(args.log, source, js_parser.OptionsFromConfig(&args.options), expr, "") - ast.URLForCSS = publicPath + ast.URLForCSS = uniqueKeyPath if pluginName != "" { result.file.inputFile.SideEffects.Kind = graph.NoSideEffects_PureData_FromPlugin } else { @@ -320,8 +304,9 @@ func parseFile(args parseArgs) { // Copy the file using an additional file payload to make sure we only copy // the file if the module isn't removed due to tree shaking. + result.file.inputFile.UniqueKey = uniqueKey result.file.inputFile.AdditionalFiles = []graph.OutputFile{{ - AbsPath: args.fs.Join(args.options.AbsOutputDir, relPath), + AbsPath: source.KeyPath.Text, Contents: []byte(source.Contents), JSONMetadataChunk: jsonMetadataChunk, }} @@ -945,12 +930,13 @@ func hashForFileName(hashBytes []byte) string { } type scanner struct { - log logger.Log - fs fs.FS - res resolver.Resolver - caches *cache.CacheSet - options config.Options - timer *helpers.Timer + log logger.Log + fs fs.FS + res resolver.Resolver + caches *cache.CacheSet + options config.Options + timer *helpers.Timer + uniqueKeyPrefix string // This is not guarded by a mutex because it's only ever modified by a single // thread. Note that not all results in the "results" array are necessarily @@ -1005,24 +991,25 @@ func ScanBundle( } } - s := scanner{ - log: log, - fs: fs, - res: res, - caches: caches, - options: options, - timer: timer, - results: make([]parseResult, 0, caches.SourceIndexCache.LenHint()), - visited: make(map[logger.Path]uint32), - resultChannel: make(chan parseResult), - } - // Each bundling operation gets a separate unique key uniqueKeyPrefix, err := generateUniqueKeyPrefix() if err != nil { log.AddError(nil, logger.Loc{}, fmt.Sprintf("Failed to read from randomness source: %s", err.Error())) } + s := scanner{ + log: log, + fs: fs, + res: res, + caches: caches, + options: options, + timer: timer, + results: make([]parseResult, 0, caches.SourceIndexCache.LenHint()), + visited: make(map[logger.Path]uint32), + resultChannel: make(chan parseResult), + uniqueKeyPrefix: uniqueKeyPrefix, + } + // Always start by parsing the runtime file s.results = append(s.results, parseResult{}) s.remaining++ @@ -1160,6 +1147,7 @@ func (s *scanner) maybeParseFile( results: s.resultChannel, inject: inject, skipResolve: skipResolve, + uniqueKeyPrefix: s.uniqueKeyPrefix, }) return sourceIndex @@ -1765,6 +1753,30 @@ func (s *scanner) processScannedFiles() []scannerFile { sb.WriteString("]\n }") } + // Turn all additional file paths from input paths into output paths by + // rewriting the output base directory to the output directory + for j, additionalFile := range result.file.inputFile.AdditionalFiles { + if relPath, ok := s.fs.Rel(s.options.AbsOutputBase, additionalFile.AbsPath); ok { + var hash string + + // Add a hash to the file name to prevent multiple files with the same name + // but different contents from colliding + if config.HasPlaceholder(s.options.AssetPathTemplate, config.HashPlaceholder) { + h := xxhash.New() + h.Write(additionalFile.Contents) + hash = hashForFileName(h.Sum(nil)) + } + + dir, base, ext := logger.PlatformIndependentPathDirBaseExt(relPath) + relPath = config.TemplateToString(config.SubstituteTemplate(s.options.AssetPathTemplate, config.PathPlaceholders{ + Dir: &dir, + Name: &base, + Hash: &hash, + })) + ext + result.file.inputFile.AdditionalFiles[j].AbsPath = s.fs.Join(s.options.AbsOutputDir, relPath) + } + } + s.results[i].file.jsonMetadataChunk = sb.String() } diff --git a/internal/bundler/bundler_loader_test.go b/internal/bundler/bundler_loader_test.go index a2b5b1ce91d..f8073b77cd6 100644 --- a/internal/bundler/bundler_loader_test.go +++ b/internal/bundler/bundler_loader_test.go @@ -397,6 +397,273 @@ func TestLoaderFileCommonJSAndES6(t *testing.T) { }) } +func TestLoaderFileRelativePathJS(t *testing.T) { + loader_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/src/entries/entry.js": ` + import x from '../images/image.png' + console.log(x) + `, + "/src/images/image.png": "x", + }, + entryPaths: []string{"/src/entries/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputBase: "/src", + AbsOutputDir: "/out", + ExtensionToLoader: map[string]config.Loader{ + ".js": config.LoaderJS, + ".png": config.LoaderFile, + }, + }, + }) +} + +func TestLoaderFileRelativePathCSS(t *testing.T) { + loader_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/src/entries/entry.css": ` + div { + background: url(../images/image.png); + } + `, + "/src/images/image.png": "x", + }, + entryPaths: []string{"/src/entries/entry.css"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputBase: "/src", + AbsOutputDir: "/out", + ExtensionToLoader: map[string]config.Loader{ + ".css": config.LoaderCSS, + ".png": config.LoaderFile, + }, + }, + }) +} + +func TestLoaderFileRelativePathAssetNamesJS(t *testing.T) { + loader_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/src/entries/entry.js": ` + import x from '../images/image.png' + console.log(x) + `, + "/src/images/image.png": "x", + }, + entryPaths: []string{"/src/entries/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputBase: "/src", + AbsOutputDir: "/out", + AssetPathTemplate: []config.PathTemplate{ + {Data: "", Placeholder: config.DirPlaceholder}, + {Data: "/", Placeholder: config.NamePlaceholder}, + {Data: "-", Placeholder: config.HashPlaceholder}, + }, + ExtensionToLoader: map[string]config.Loader{ + ".js": config.LoaderJS, + ".png": config.LoaderFile, + }, + }, + }) +} + +func TestLoaderFileRelativePathAssetNamesCSS(t *testing.T) { + loader_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/src/entries/entry.css": ` + div { + background: url(../images/image.png); + } + `, + "/src/images/image.png": "x", + }, + entryPaths: []string{"/src/entries/entry.css"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputBase: "/src", + AbsOutputDir: "/out", + AssetPathTemplate: []config.PathTemplate{ + {Data: "", Placeholder: config.DirPlaceholder}, + {Data: "/", Placeholder: config.NamePlaceholder}, + {Data: "-", Placeholder: config.HashPlaceholder}, + }, + ExtensionToLoader: map[string]config.Loader{ + ".css": config.LoaderCSS, + ".png": config.LoaderFile, + }, + }, + }) +} + +func TestLoaderFilePublicPathJS(t *testing.T) { + loader_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/src/entries/entry.js": ` + import x from '../images/image.png' + console.log(x) + `, + "/src/images/image.png": "x", + }, + entryPaths: []string{"/src/entries/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputBase: "/src", + AbsOutputDir: "/out", + PublicPath: "https://example.com", + ExtensionToLoader: map[string]config.Loader{ + ".js": config.LoaderJS, + ".png": config.LoaderFile, + }, + }, + }) +} + +func TestLoaderFilePublicPathCSS(t *testing.T) { + loader_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/src/entries/entry.css": ` + div { + background: url(../images/image.png); + } + `, + "/src/images/image.png": "x", + }, + entryPaths: []string{"/src/entries/entry.css"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputBase: "/src", + AbsOutputDir: "/out", + PublicPath: "https://example.com", + ExtensionToLoader: map[string]config.Loader{ + ".css": config.LoaderCSS, + ".png": config.LoaderFile, + }, + }, + }) +} + +func TestLoaderFilePublicPathAssetNamesJS(t *testing.T) { + loader_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/src/entries/entry.js": ` + import x from '../images/image.png' + console.log(x) + `, + "/src/images/image.png": "x", + }, + entryPaths: []string{"/src/entries/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputBase: "/src", + AbsOutputDir: "/out", + PublicPath: "https://example.com", + AssetPathTemplate: []config.PathTemplate{ + {Data: "", Placeholder: config.DirPlaceholder}, + {Data: "/", Placeholder: config.NamePlaceholder}, + {Data: "-", Placeholder: config.HashPlaceholder}, + }, + ExtensionToLoader: map[string]config.Loader{ + ".js": config.LoaderJS, + ".png": config.LoaderFile, + }, + }, + }) +} + +func TestLoaderFilePublicPathAssetNamesCSS(t *testing.T) { + loader_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/src/entries/entry.css": ` + div { + background: url(../images/image.png); + } + `, + "/src/images/image.png": "x", + }, + entryPaths: []string{"/src/entries/entry.css"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputBase: "/src", + AbsOutputDir: "/out", + PublicPath: "https://example.com", + AssetPathTemplate: []config.PathTemplate{ + {Data: "", Placeholder: config.DirPlaceholder}, + {Data: "/", Placeholder: config.NamePlaceholder}, + {Data: "-", Placeholder: config.HashPlaceholder}, + }, + ExtensionToLoader: map[string]config.Loader{ + ".css": config.LoaderCSS, + ".png": config.LoaderFile, + }, + }, + }) +} + +func TestLoaderFileOneSourceTwoDifferentOutputPathsJS(t *testing.T) { + loader_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/src/entries/entry.js": ` + import '../shared/common.js' + `, + "/src/entries/other/entry.js": ` + import '../../shared/common.js' + `, + "/src/shared/common.js": ` + import x from './common.png' + console.log(x) + `, + "/src/shared/common.png": "x", + }, + entryPaths: []string{ + "/src/entries/entry.js", + "/src/entries/other/entry.js", + }, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputBase: "/src", + AbsOutputDir: "/out", + ExtensionToLoader: map[string]config.Loader{ + ".js": config.LoaderJS, + ".png": config.LoaderFile, + }, + }, + }) +} + +func TestLoaderFileOneSourceTwoDifferentOutputPathsCSS(t *testing.T) { + loader_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/src/entries/entry.css": ` + @import "../shared/common.css"; + `, + "/src/entries/other/entry.css": ` + @import "../../shared/common.css"; + `, + "/src/shared/common.css": ` + div { + background: url(common.png); + } + `, + "/src/shared/common.png": "x", + }, + entryPaths: []string{ + "/src/entries/entry.css", + "/src/entries/other/entry.css", + }, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputBase: "/src", + AbsOutputDir: "/out", + ExtensionToLoader: map[string]config.Loader{ + ".css": config.LoaderCSS, + ".png": config.LoaderFile, + }, + }, + }) +} + func TestLoaderJSONNoBundle(t *testing.T) { loader_suite.expectBundled(t, bundled{ files: map[string]string{ diff --git a/internal/bundler/linker.go b/internal/bundler/linker.go index e0da426f66f..41a7a32fd63 100644 --- a/internal/bundler/linker.go +++ b/internal/bundler/linker.go @@ -121,6 +121,7 @@ type outputPieceIndexKind uint8 const ( outputPieceNone outputPieceIndexKind = iota + outputPieceAssetIndex outputPieceChunkIndex ) @@ -537,6 +538,18 @@ func (c *linkerContext) substituteFinalPaths( shift.After.Add(dataOffset) switch piece.kind { + case outputPieceAssetIndex: + file := c.graph.Files[piece.index] + if len(file.InputFile.AdditionalFiles) != 1 { + panic("Internal error") + } + relPath, _ := c.fs.Rel(c.options.AbsOutputDir, file.InputFile.AdditionalFiles[0].AbsPath) + importPath := modifyPath(relPath) + j.AddString(importPath) + shift.Before.AdvanceString(file.InputFile.UniqueKey) + shift.After.AdvanceString(importPath) + shifts = append(shifts, shift) + case outputPieceChunkIndex: chunk := chunks[piece.index] importPath := modifyPath(chunk.finalRelPath) @@ -1057,7 +1070,8 @@ func (a stableRefArray) Len() int { return len(a) } func (a stableRefArray) Swap(i int, j int) { a[i], a[j] = a[j], a[i] } func (a stableRefArray) Less(i int, j int) bool { ai, aj := a[i], a[j] - return ai.StableSourceIndex < aj.StableSourceIndex || (ai.StableSourceIndex == aj.StableSourceIndex && ai.Ref.InnerIndex < aj.Ref.InnerIndex) + return ai.StableSourceIndex < aj.StableSourceIndex || + (ai.StableSourceIndex == aj.StableSourceIndex && ai.Ref.InnerIndex < aj.Ref.InnerIndex) } // Sort cross-chunk exports by chunk name for determinism @@ -4978,6 +4992,8 @@ func (c *linkerContext) breakOutputIntoPieces(j helpers.Joiner, chunkCount uint3 boundary = -1 } else { switch output[start] { + case 'A': + kind = outputPieceAssetIndex case 'C': kind = outputPieceChunkIndex } @@ -4994,10 +5010,16 @@ func (c *linkerContext) breakOutputIntoPieces(j helpers.Joiner, chunkCount uint3 // Validate the boundary switch kind { + case outputPieceAssetIndex: + if index >= uint32(len(c.graph.Files)) { + boundary = -1 + } + case outputPieceChunkIndex: if index >= chunkCount { boundary = -1 } + default: boundary = -1 } diff --git a/internal/bundler/snapshots/snapshots_loader.txt b/internal/bundler/snapshots/snapshots_loader.txt index 33e270f4c4c..00fbc33935d 100644 --- a/internal/bundler/snapshots/snapshots_loader.txt +++ b/internal/bundler/snapshots/snapshots_loader.txt @@ -138,6 +138,128 @@ var require_test2 = __commonJS({ // entry.js console.log(require_test(), require_test2()); +================================================================================ +TestLoaderFileOneSourceTwoDifferentOutputPathsCSS +---------- /out/common-LSAMBFUD.png ---------- +x +---------- /out/entries/entry.css ---------- +/* src/shared/common.css */ +div { + background: url(../common-LSAMBFUD.png); +} + +/* src/entries/entry.css */ + +---------- /out/entries/other/entry.css ---------- +/* src/shared/common.css */ +div { + background: url(../../common-LSAMBFUD.png); +} + +/* src/entries/other/entry.css */ + +================================================================================ +TestLoaderFileOneSourceTwoDifferentOutputPathsJS +---------- /out/common-LSAMBFUD.png ---------- +x +---------- /out/entries/entry.js ---------- +// src/shared/common.png +var common_default = "../common-LSAMBFUD.png"; + +// src/shared/common.js +console.log(common_default); + +---------- /out/entries/other/entry.js ---------- +// src/shared/common.png +var common_default = "../../common-LSAMBFUD.png"; + +// src/shared/common.js +console.log(common_default); + +================================================================================ +TestLoaderFilePublicPathAssetNamesCSS +---------- /out/images/image-LSAMBFUD.png ---------- +x +---------- /out/entries/entry.css ---------- +/* src/entries/entry.css */ +div { + background: url(https://example.com/images/image-LSAMBFUD.png); +} + +================================================================================ +TestLoaderFilePublicPathAssetNamesJS +---------- /out/images/image-LSAMBFUD.png ---------- +x +---------- /out/entries/entry.js ---------- +// src/images/image.png +var image_default = "https://example.com/images/image-LSAMBFUD.png"; + +// src/entries/entry.js +console.log(image_default); + +================================================================================ +TestLoaderFilePublicPathCSS +---------- /out/image-LSAMBFUD.png ---------- +x +---------- /out/entries/entry.css ---------- +/* src/entries/entry.css */ +div { + background: url(https://example.com/image-LSAMBFUD.png); +} + +================================================================================ +TestLoaderFilePublicPathJS +---------- /out/image-LSAMBFUD.png ---------- +x +---------- /out/entries/entry.js ---------- +// src/images/image.png +var image_default = "https://example.com/image-LSAMBFUD.png"; + +// src/entries/entry.js +console.log(image_default); + +================================================================================ +TestLoaderFileRelativePathAssetNamesCSS +---------- /out/images/image-LSAMBFUD.png ---------- +x +---------- /out/entries/entry.css ---------- +/* src/entries/entry.css */ +div { + background: url(../images/image-LSAMBFUD.png); +} + +================================================================================ +TestLoaderFileRelativePathAssetNamesJS +---------- /out/images/image-LSAMBFUD.png ---------- +x +---------- /out/entries/entry.js ---------- +// src/images/image.png +var image_default = "../images/image-LSAMBFUD.png"; + +// src/entries/entry.js +console.log(image_default); + +================================================================================ +TestLoaderFileRelativePathCSS +---------- /out/image-LSAMBFUD.png ---------- +x +---------- /out/entries/entry.css ---------- +/* src/entries/entry.css */ +div { + background: url(../image-LSAMBFUD.png); +} + +================================================================================ +TestLoaderFileRelativePathJS +---------- /out/image-LSAMBFUD.png ---------- +x +---------- /out/entries/entry.js ---------- +// src/images/image.png +var image_default = "../image-LSAMBFUD.png"; + +// src/entries/entry.js +console.log(image_default); + ================================================================================ TestLoaderJSONCommonJSAndES6 ---------- /out.js ---------- diff --git a/internal/graph/input.go b/internal/graph/input.go index 90dc5c75c0c..6e707c48a70 100644 --- a/internal/graph/input.go +++ b/internal/graph/input.go @@ -26,6 +26,7 @@ type InputFile struct { // that must be written to the output directory. It's used by the "file" // loader. AdditionalFiles []OutputFile + UniqueKey string SideEffects SideEffects Loader config.Loader diff --git a/scripts/js-api-tests.js b/scripts/js-api-tests.js index 899987f1835..33bb06d3da4 100644 --- a/scripts/js-api-tests.js +++ b/scripts/js-api-tests.js @@ -1005,7 +1005,7 @@ body { assert.deepStrictEqual(json.outputs[outImport2].exports, []) assert.deepStrictEqual(json.outputs[outChunk].exports, []) - assert.deepStrictEqual(json.outputs[outEntry].inputs, { [inEntry]: { bytesInOutput: 72 } }) + assert.deepStrictEqual(json.outputs[outEntry].inputs, { [inEntry]: { bytesInOutput: 74 } }) assert.deepStrictEqual(json.outputs[outImport1].inputs, { [inImport1]: { bytesInOutput: 0 } }) assert.deepStrictEqual(json.outputs[outImport2].inputs, { [inImport2]: { bytesInOutput: 0 } }) assert.deepStrictEqual(json.outputs[outChunk].inputs, { [inShared]: { bytesInOutput: 28 } })