From 91cf8370f32f4bfe8bd5724e6cd486c0ca6c72f2 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Wed, 17 Feb 2021 02:29:10 -0800 Subject: [PATCH] fix #826: sometimes ignore path query part --- CHANGELOG.md | 17 ++++++ internal/bundler/bundler.go | 2 +- internal/bundler/bundler_default_test.go | 59 +++++++++++++++++++ internal/bundler/bundler_loader_test.go | 45 ++++++++++++++ .../bundler/snapshots/snapshots_default.txt | 59 +++++++++++++++++++ internal/logger/logger.go | 12 +++- internal/resolver/resolver.go | 31 +++++++--- 7 files changed, 214 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 557d598c5e8..112b42851e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,23 @@ However, this warning is not relevant when accessing a property off of the `require` object such as `require.cache` because a property access does not result in capturing the value of `require`. Now a warning is no longer generated for `require.someProperty` when the output format is `cjs`. This allows for the use of features such as `require.cache` and `require.extensions`. This fix was contributed by [@huonw](https://github.com/huonw). +* Support ignored URL parameters at the end of import paths ([#826](https://github.com/evanw/esbuild/issues/826)) + + If path resolution fails, ebuild will now try again with the URL query and/or fragment removed. This helps handle ancient CSS code like this that contains hacks for Internet Explorer: + + ```css + @font-face { + src: + url("./themes/default/assets/fonts/icons.eot?#iefix") format('embedded-opentype'), + url("./themes/default/assets/fonts/icons.woff2") format('woff2'), + url("./themes/default/assets/fonts/icons.woff") format('woff'), + url("./themes/default/assets/fonts/icons.ttf") format('truetype'), + url("./themes/default/assets/fonts/icons.svg#icons") format('svg'); + } + ``` + + Previously path resolution would fail because these files do not end with the `.eot?#iefix` or `.svg#icons` extensions. Now path resolution should succeed. The URL query and fragment are not unconditionally stripped because there is apparently [code in the wild that uses `#` as a directory name](https://github.com/medikoo/es5-ext/tree/3ddd2066b19e7c25a782869a304ae35d8188c8f1/string/%23). So esbuild will still try to resolve the full import path first and only try to reinterpret the path as a URL if that fails. + ## 0.8.46 * Fix minification of `.0` in CSS ([#804](https://github.com/evanw/esbuild/issues/804)) diff --git a/internal/bundler/bundler.go b/internal/bundler/bundler.go index 27a09a96809..a42e91ff413 100644 --- a/internal/bundler/bundler.go +++ b/internal/bundler/bundler.go @@ -304,7 +304,7 @@ func parseFile(args parseArgs) { // but different contents from colliding hash := hashForFileName([]byte(source.Contents)) additionalFileName := base + "." + hash + ext - publicPath := args.options.PublicPath + additionalFileName + publicPath := args.options.PublicPath + additionalFileName + source.KeyPath.IgnoredSuffix // Determine the destination folder targetFolder := args.options.AbsOutputDir diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index dd2f8e6da0b..501f7a694c5 100644 --- a/internal/bundler/bundler_default_test.go +++ b/internal/bundler/bundler_default_test.go @@ -2247,6 +2247,65 @@ func TestExternalModuleExclusionRelativePath(t *testing.T) { }) } +// Webpack supports this case, so we do too. Some libraries apparently have +// these paths: https://github.com/webpack/enhanced-resolve/issues/247 +func TestImportWithHashInPath(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + import foo from './file#foo.txt' + import bar from './file#bar.txt' + console.log(foo, bar) + `, + "/file#foo.txt": `foo`, + "/file#bar.txt": `bar`, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + }, + }) +} + +func TestImportWithHashParameter(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + // Each of these should have a separate identity (i.e. end up in the output file twice) + import foo from './file.txt#foo' + import bar from './file.txt#bar' + console.log(foo, bar) + `, + "/file.txt": `This is some text`, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + }, + }) +} + +func TestImportWithQueryParameter(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + // Each of these should have a separate identity (i.e. end up in the output file twice) + import foo from './file.txt?foo' + import bar from './file.txt?bar' + console.log(foo, bar) + `, + "/file.txt": `This is some text`, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + }, + }) +} + func TestAutoExternal(t *testing.T) { default_suite.expectBundled(t, bundled{ files: map[string]string{ diff --git a/internal/bundler/bundler_loader_test.go b/internal/bundler/bundler_loader_test.go index 38a7c0e8bfa..d5bd10dfc3f 100644 --- a/internal/bundler/bundler_loader_test.go +++ b/internal/bundler/bundler_loader_test.go @@ -401,3 +401,48 @@ func TestLoaderJSONSharedWithMultipleEntriesIssue413(t *testing.T) { }, }) } + +func TestLoaderFileWithQueryParameter(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + // Each of these should have a separate identity (i.e. end up in the output file twice) + import foo from './file.txt?foo' + import bar from './file.txt?bar' + console.log(foo, bar) + `, + "/file.txt": `This is some text`, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + ExtensionToLoader: map[string]config.Loader{ + ".js": config.LoaderJS, + ".txt": config.LoaderFile, + }, + }, + }) +} + +func TestLoaderFromExtensionWithQueryParameter(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + import foo from './file.abc?query.xyz' + console.log(foo) + `, + "/file.abc": `This should not be base64 encoded`, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + ExtensionToLoader: map[string]config.Loader{ + ".js": config.LoaderJS, + ".abc": config.LoaderText, + ".xyz": config.LoaderBase64, + }, + }, + }) +} diff --git a/internal/bundler/snapshots/snapshots_default.txt b/internal/bundler/snapshots/snapshots_default.txt index e09a10ed9c0..2b64fa0bd06 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/internal/bundler/snapshots/snapshots_default.txt @@ -1004,6 +1004,42 @@ var Internal = () => /* @__PURE__ */ h(p, null, " Test 2 "); var App = () => /* @__PURE__ */ h(p, null, " ", /* @__PURE__ */ h(Internal, null), " T "); render(/* @__PURE__ */ h(App, null), document.getElementById("app")); +================================================================================ +TestImportWithHashInPath +---------- /out/entry.js ---------- +// file#foo.txt +var file_foo_default = "foo"; + +// file#bar.txt +var file_bar_default = "bar"; + +// entry.js +console.log(file_foo_default, file_bar_default); + +================================================================================ +TestImportWithHashParameter +---------- /out/entry.js ---------- +// file.txt#foo +var file_default = "This is some text"; + +// file.txt#bar +var file_default2 = "This is some text"; + +// entry.js +console.log(file_default, file_default2); + +================================================================================ +TestImportWithQueryParameter +---------- /out/entry.js ---------- +// file.txt?foo +var file_default = "This is some text"; + +// file.txt?bar +var file_default2 = "This is some text"; + +// entry.js +console.log(file_default, file_default2); + ================================================================================ TestInject ---------- /out.js ---------- @@ -1123,6 +1159,29 @@ var clsExprKeep = /* @__PURE__ */ __name(class { }, "keep"); new clsExprKeep(); +================================================================================ +TestLoaderFileWithQueryParameter +---------- /out/file.JAWLBT6L.txt ---------- +This is some text +---------- /out/entry.js ---------- +// file.txt?foo +var file_default = "file.JAWLBT6L.txt?foo"; + +// file.txt?bar +var file_default2 = "file.JAWLBT6L.txt?bar"; + +// entry.js +console.log(file_default, file_default2); + +================================================================================ +TestLoaderFromExtensionWithQueryParameter +---------- /out/entry.js ---------- +// file.abc?query.xyz +var file_default = "This should not be base64 encoded"; + +// entry.js +console.log(file_default); + ================================================================================ TestManyEntryPoints ---------- /out/e00.js ---------- diff --git a/internal/logger/logger.go b/internal/logger/logger.go index c13b523ad2b..89ca9e149f3 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -129,7 +129,14 @@ func (a SortableMsgs) Less(i int, j int) bool { type Path struct { Text string Namespace string - Flags PathFlags + + // This feature was added to support ancient CSS libraries that append things + // like "?#iefix" and "#icons" to some of their import paths as a hack for IE6. + // The intent is for these suffix parts to be ignored but passed through to + // the output. This is supported by other bundlers, so we also support this. + IgnoredSuffix string + + Flags PathFlags } type PathFlags uint8 @@ -146,7 +153,8 @@ func (p Path) IsDisabled() bool { func (a Path) ComesBeforeInSortedOrder(b Path) bool { return a.Namespace > b.Namespace || (a.Namespace == b.Namespace && (a.Text < b.Text || - (a.Text == b.Text && a.Flags < b.Flags))) + (a.Text == b.Text && (a.Flags < b.Flags || + (a.Flags == b.Flags && a.IgnoredSuffix < b.IgnoredSuffix))))) } // This has a custom implementation instead of using "filepath.Dir/Base/Ext" diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go index c7c8aaff21d..b8c441fcd68 100644 --- a/internal/resolver/resolver.go +++ b/internal/resolver/resolver.go @@ -226,11 +226,24 @@ func (r *resolver) Resolve(sourceDir string, importPath string, kind ast.ImportK result := r.resolveWithoutSymlinks(sourceDir, importPath, kind) if result == nil { - return nil + // If resolution failed, try again with the URL query and/or hash removed + suffix := strings.IndexAny(importPath, "?#") + if suffix < 1 { + return nil + } + result = r.resolveWithoutSymlinks(sourceDir, importPath[:suffix], kind) + if result == nil { + return nil + } + result.PathPair.Primary.IgnoredSuffix = importPath[suffix:] + if result.PathPair.HasSecondary() { + result.PathPair.Secondary.IgnoredSuffix = importPath[suffix:] + } } // If successful, resolve symlinks using the directory info cache - return r.finalizeResolve(*result) + r.finalizeResolve(result) + return result } func (r *resolver) isExternalPattern(path string) bool { @@ -249,7 +262,9 @@ func (r *resolver) ResolveAbs(absPath string) *ResolveResult { defer r.mutex.Unlock() // Just decorate the absolute path with information from parent directories - return r.finalizeResolve(ResolveResult{PathPair: PathPair{Primary: logger.Path{Text: absPath, Namespace: "file"}}}) + result := &ResolveResult{PathPair: PathPair{Primary: logger.Path{Text: absPath, Namespace: "file"}}} + r.finalizeResolve(result) + return result } func (r *resolver) ProbeResolvePackageAsRelative(sourceDir string, importPath string, kind ast.ImportKind) *ResolveResult { @@ -259,7 +274,9 @@ func (r *resolver) ProbeResolvePackageAsRelative(sourceDir string, importPath st defer r.mutex.Unlock() if pair, ok := r.loadAsFileOrDirectory(absPath, kind); ok { - return r.finalizeResolve(ResolveResult{PathPair: pair}) + result := &ResolveResult{PathPair: pair} + r.finalizeResolve(result) + return result } return nil } @@ -283,7 +300,7 @@ func IsInsideNodeModules(fs fs.FS, path string) bool { } } -func (r *resolver) finalizeResolve(result ResolveResult) *ResolveResult { +func (r *resolver) finalizeResolve(result *ResolveResult) { for _, path := range result.PathPair.iter() { if path.Namespace == "file" { if dirInfo := r.dirInfoCached(r.fs.Dir(path.Text)); dirInfo != nil { @@ -346,8 +363,6 @@ func (r *resolver) finalizeResolve(result ResolveResult) *ResolveResult { } } } - - return &result } func (r *resolver) resolveWithoutSymlinks(sourceDir string, importPath string, kind ast.ImportKind) *ResolveResult { @@ -527,7 +542,7 @@ func (r *resolver) PrettyPath(path logger.Path) string { path.Text = "(disabled):" + path.Text } - return path.Text + return path.Text + path.IgnoredSuffix } ////////////////////////////////////////////////////////////////////////////////