From e79a747bf087da4e838bc7e8816c1f423b1d4832 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Sat, 24 Apr 2021 03:33:42 -0700 Subject: [PATCH] fix #1184: implicit "**/" in "sideEffects" --- CHANGELOG.md | 4 +++ internal/resolver/package_json.go | 57 +++++++++++++++++++++++++------ scripts/end-to-end-tests.js | 12 +++++++ 3 files changed, 63 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bf2f20dd43..2a20e1f008a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,10 @@ However, doing this is very un-ergonomic and exporting something as an arbitrary name is impossible outside of `export * from`. So this proposal is designed to fully fill out the possibility matrix and make arbitrary alias names a proper first-class feature. +* Implement more accurate `sideEffects` behavior from Webpack ([#1184](https://github.com/evanw/esbuild/issues/1184)) + + This release adds support for the implicit `**/` prefix that must be added to paths in the `sideEffects` array in `package.json` if the path does not contain `/`. Another way of saying this is if `package.json` contains a `sideEffects` array with a string that doesn't contain a `/` then it should be treated as a file name instead of a path. Previously esbuild treated all strings in this array as paths, which does not match how Webpack behaves. The result of this meant that esbuild could consider files to have no side effects while Webpack would consider the same files to have side effects. This bug should now be fixed. + ## 0.11.13 * Implement ergonomic brand checks for private fields diff --git a/internal/resolver/package_json.go b/internal/resolver/package_json.go index 2020ac0cf0b..70e855e2322 100644 --- a/internal/resolver/package_json.go +++ b/internal/resolver/package_json.go @@ -260,8 +260,13 @@ func (r resolverQuery) parsePackageJSON(inputPath string) *packageJSON { continue } - absPattern := r.fs.Join(inputPath, js_lexer.UTF16ToString(item.Value)) - re, hadWildcard := globToEscapedRegexp(absPattern) + // Reference: https://github.com/webpack/webpack/blob/ed175cd22f89eb9fecd0a70572a3fd0be028e77c/lib/optimize/SideEffectsFlagPlugin.js + pattern := js_lexer.UTF16ToString(item.Value) + if !strings.ContainsRune(pattern, '/') { + pattern = "**/" + pattern + } + absPattern := r.fs.Join(inputPath, pattern) + re, hadWildcard := globstarToEscapedRegexp(absPattern) // Wildcard patterns require more expensive matching if hadWildcard { @@ -290,27 +295,59 @@ func (r resolverQuery) parsePackageJSON(inputPath string) *packageJSON { return packageJSON } -func globToEscapedRegexp(glob string) (string, bool) { +// Reference: https://github.com/fitzgen/glob-to-regexp/blob/2abf65a834259c6504ed3b80e85f893f8cd99127/index.js +func globstarToEscapedRegexp(glob string) (string, bool) { sb := strings.Builder{} sb.WriteByte('^') hadWildcard := false + n := len(glob) - for _, c := range glob { + for i := 0; i < n; i++ { + c := glob[i] switch c { case '\\', '^', '$', '.', '+', '|', '(', ')', '[', ']', '{', '}': sb.WriteByte('\\') - sb.WriteRune(c) - - case '*': - sb.WriteString(".*") - hadWildcard = true + sb.WriteByte(c) case '?': sb.WriteByte('.') hadWildcard = true + case '*': + // Move over all consecutive "*"'s. + // Also store the previous and next characters + prevChar := -1 + if i > 0 { + prevChar = int(glob[i-1]) + } + starCount := 1 + for i+1 < n && glob[i+1] == '*' { + starCount++ + i++ + } + nextChar := -1 + if i+1 < n { + nextChar = int(glob[i+1]) + } + + // Determine if this is a globstar segment + isGlobstar := starCount > 1 && // multiple "*"'s + (prevChar == '/' || prevChar == -1) && // from the start of the segment + (nextChar == '/' || nextChar == -1) // to the end of the segment + + if isGlobstar { + // It's a globstar, so match zero or more path segments + sb.WriteString("(?:[^/]*(?:/|$))*") + i++ // Move over the "/" + } else { + // It's not a globstar, so only match one path segment + sb.WriteString("[^/]*") + } + + hadWildcard = true + default: - sb.WriteRune(c) + sb.WriteByte(c) } } diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index eb9a0b7a28e..bc6a9ae6e93 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -2349,6 +2349,18 @@ 'entry.js': `import {foo, bar} from './foo'; let unused = foo; if (bar) throw 'expected "foo" to be tree-shaken'`, 'foo.js': `module.exports = {get foo() { module.exports.bar = 1 }, bar: 0}`, }), + + // Test for an implicit and explicit "**/" prefix (see https://github.com/evanw/esbuild/issues/1184) + test(['--bundle', 'entry.js', '--outfile=node.js'], { + 'entry.js': `import './foo'; if (global.dce6 !== 123) throw 'fail'`, + 'foo/dir/x.js': `global.dce6 = 123`, + 'foo/package.json': `{ "main": "dir/x", "sideEffects": ["x.*"] }`, + }), + test(['--bundle', 'entry.js', '--outfile=node.js'], { + 'entry.js': `import './foo'; if (global.dce6 !== 123) throw 'fail'`, + 'foo/dir/x.js': `global.dce6 = 123`, + 'foo/package.json': `{ "main": "dir/x", "sideEffects": ["**/x.*"] }`, + }), ) // Test obscure CommonJS symbol edge cases