Skip to content

Commit

Permalink
fix #4053: reordering of .tsx in node_modules
Browse files Browse the repository at this point in the history
  • Loading branch information
evanw committed Feb 6, 2025
1 parent dc71977 commit 4cdf03c
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 30 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@

With this release, you can now use BigInt literals as define values, such as with `--define:FOO=123n`. Previously trying to do this resulted in a syntax error.

* Fix a bug with resolve extensions in `node_modules` ([#4053](/~https://github.com/evanw/esbuild/issues/4053))

The `--resolve-extensions=` option lets you specify the order in which to try resolving implicit file extensions. For complicated reasons, esbuild reorders TypeScript file extensions after JavaScript ones inside of `node_modules` so that JavaScript source code is always preferred to TypeScript source code inside of dependencies. However, this reordering had a bug that could accidentally change the relative order of TypeScript file extensions if one of them was a prefix of the other. That bug has been fixed in this release. You can see the issue for details.

* Emit `/* @__KEY__ */` for string literals derived from property names ([#4034](/~https://github.com/evanw/esbuild/issues/4034))

Property name mangling is an advanced feature that shortens certain property names for better minification (I say "advanced feature" because it's very easy to break your code with it). Sometimes you need to store a property name in a string, such as `obj.get('foo')` instead of `obj.foo`. JavaScript minifiers such as esbuild and [Terser](https://terser.org/) have a convention where a `/* @__KEY__ */` comment before the string makes it behave like a property name. So `obj.get(/* @__KEY__ */ 'foo')` allows the contents of the string `'foo'` to be shortened.
Expand Down
26 changes: 1 addition & 25 deletions internal/bundler/bundler.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ func parseFile(args parseArgs) {

// The special "default" loader determines the loader from the file path
if loader == config.LoaderDefault {
loader = loaderFromFileExtension(args.options.ExtensionToLoader, base+ext)
loader = config.LoaderFromFileExtension(args.options.ExtensionToLoader, base+ext)
}

// Reject unsupported import attributes when the loader isn't "copy" (since
Expand Down Expand Up @@ -1199,30 +1199,6 @@ func runOnLoadPlugins(
return loaderPluginResult{loader: config.LoaderNone}, true
}

func loaderFromFileExtension(extensionToLoader map[string]config.Loader, base string) config.Loader {
// Pick the loader with the longest matching extension. So if there's an
// extension for ".css" and for ".module.css", we want to match the one for
// ".module.css" before the one for ".css".
if i := strings.IndexByte(base, '.'); i != -1 {
for {
if loader, ok := extensionToLoader[base[i:]]; ok {
return loader
}
base = base[i+1:]
i = strings.IndexByte(base, '.')
if i == -1 {
break
}
}
} else {
// If there's no extension, explicitly check for an extensionless loader
if loader, ok := extensionToLoader[""]; ok {
return loader
}
}
return config.LoaderNone
}

// Identify the path by its lowercase absolute path name with Windows-specific
// slashes substituted for standard slashes. This should hopefully avoid path
// issues on Windows where multiple different paths can refer to the same
Expand Down
40 changes: 40 additions & 0 deletions internal/bundler_tests/bundler_default_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9206,3 +9206,43 @@ func TestSourceIdentifierNameIndexMultipleEntry(t *testing.T) {
},
})
}

func TestResolveExtensionsOrderIssue4053(t *testing.T) {
css_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
import test from "./Test"
import image from "expo-image"
console.log(test === 'Test.web.tsx')
console.log(image === 'Image.web.tsx')
`,
"/Test.web.tsx": `export default 'Test.web.tsx'`,
"/Test.tsx": `export default 'Test.tsx'`,
"/node_modules/expo-image/index.js": `
export { default } from "./Image"
`,
"/node_modules/expo-image/Image.web.tsx": `export default 'Image.web.tsx'`,
"/node_modules/expo-image/Image.tsx": `export default 'Image.tsx'`,
},
entryPaths: []string{"/entry.js"},
options: config.Options{
Mode: config.ModeBundle,
AbsOutputFile: "/out.js",
ExtensionOrder: []string{
".web.mjs",
".mjs",
".web.js",
".js",
".web.mts",
".mts",
".web.ts",
".ts",
".web.jsx",
".jsx",
".web.tsx",
".tsx",
".json",
},
},
})
}
13 changes: 13 additions & 0 deletions internal/bundler_tests/snapshots/snapshots_css.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3443,6 +3443,19 @@ c {
background: url(data:image/png;base64,Yy0z);
}

================================================================================
TestResolveExtensionsOrderIssue4053
---------- /out.js ----------
// Test.web.tsx
var Test_web_default = "Test.web.tsx";

// node_modules/expo-image/Image.web.tsx
var Image_web_default = "Image.web.tsx";

// entry.js
console.log(Test_web_default === "Test.web.tsx");
console.log(Image_web_default === "Image.web.tsx");

================================================================================
TestTextImportURLInCSSText
---------- /out/entry.css ----------
Expand Down
24 changes: 24 additions & 0 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,30 @@ func (loader Loader) CanHaveSourceMap() bool {
return false
}

func LoaderFromFileExtension(extensionToLoader map[string]Loader, base string) Loader {
// Pick the loader with the longest matching extension. So if there's an
// extension for ".css" and for ".module.css", we want to match the one for
// ".module.css" before the one for ".css".
if i := strings.IndexByte(base, '.'); i != -1 {
for {
if loader, ok := extensionToLoader[base[i:]]; ok {
return loader
}
base = base[i+1:]
i = strings.IndexByte(base, '.')
if i == -1 {
break
}
}
} else {
// If there's no extension, explicitly check for an extensionless loader
if loader, ok := extensionToLoader[""]; ok {
return loader
}
}
return LoaderNone
}

type Format uint8

const (
Expand Down
10 changes: 5 additions & 5 deletions internal/resolver/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ func NewResolver(call config.APICall, fs fs.FS, log logger.Log, caches *cache.Ca
// Filter out non-CSS extensions for CSS "@import" imports
cssExtensionOrder := make([]string, 0, len(options.ExtensionOrder))
for _, ext := range options.ExtensionOrder {
if loader, ok := options.ExtensionToLoader[ext]; !ok || loader.IsCSS() {
if loader := config.LoaderFromFileExtension(options.ExtensionToLoader, ext); loader == config.LoaderNone || loader.IsCSS() {
cssExtensionOrder = append(cssExtensionOrder, ext)
}
}
Expand All @@ -257,23 +257,23 @@ func NewResolver(call config.APICall, fs fs.FS, log logger.Log, caches *cache.Ca
nodeModulesExtensionOrder := make([]string, 0, len(options.ExtensionOrder))
split := 0
for i, ext := range options.ExtensionOrder {
if loader, ok := options.ExtensionToLoader[ext]; ok && loader == config.LoaderJS || loader == config.LoaderJSX {
if loader := config.LoaderFromFileExtension(options.ExtensionToLoader, ext); loader == config.LoaderJS || loader == config.LoaderJSX {
split = i + 1 // Split after the last JavaScript extension
}
}
if split != 0 { // Only do this if there are any JavaScript extensions
for _, ext := range options.ExtensionOrder[:split] { // Non-TypeScript extensions before the split
if loader, ok := options.ExtensionToLoader[ext]; !ok || !loader.IsTypeScript() {
if loader := config.LoaderFromFileExtension(options.ExtensionToLoader, ext); !loader.IsTypeScript() {
nodeModulesExtensionOrder = append(nodeModulesExtensionOrder, ext)
}
}
for _, ext := range options.ExtensionOrder { // All TypeScript extensions
if loader, ok := options.ExtensionToLoader[ext]; ok && loader.IsTypeScript() {
if loader := config.LoaderFromFileExtension(options.ExtensionToLoader, ext); loader.IsTypeScript() {
nodeModulesExtensionOrder = append(nodeModulesExtensionOrder, ext)
}
}
for _, ext := range options.ExtensionOrder[split:] { // Non-TypeScript extensions after the split
if loader, ok := options.ExtensionToLoader[ext]; !ok || !loader.IsTypeScript() {
if loader := config.LoaderFromFileExtension(options.ExtensionToLoader, ext); !loader.IsTypeScript() {
nodeModulesExtensionOrder = append(nodeModulesExtensionOrder, ext)
}
}
Expand Down

0 comments on commit 4cdf03c

Please sign in to comment.