Skip to content

Commit

Permalink
revert: "fix: use string manipulation instead of regex to inject esbu…
Browse files Browse the repository at this point in the history
…ild helpers

(#14094)"

This reverts commit 128ad8f.
  • Loading branch information
bluwy committed Oct 5, 2023
1 parent f3a3e77 commit 54e1275
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 30 deletions.
39 changes: 16 additions & 23 deletions packages/vite/src/node/plugins/esbuild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ import { searchForWorkspaceRoot } from '../server/searchRoot'

const debug = createDebugger('vite:esbuild')

// IIFE content looks like `var MyLib = function() {`. Spaces are removed when minified
const IIFE_BEGIN_RE =
/(const|var)\s+\S+\s*=\s*function\(\)\s*\{.*"use strict";/s
const INJECT_HELPERS_IIFE_RE =
/^(.*?)((?:const|var)\s+\S+\s*=\s*function\s*\([^)]*\)\s*\{\s*"use strict";)/s
const INJECT_HELPERS_UMD_RE =
/^(.*?)(\(function\([^)]*\)\s*\{.+?amd.+?function\([^)]*\)\s*\{\s*"use strict";)/s

const validExtensionRE = /\.\w+$/
const jsxExtensionsRE = /\.(?:j|t)sx\b/
Expand Down Expand Up @@ -332,30 +333,22 @@ export const buildEsbuildPlugin = (config: ResolvedConfig): Plugin => {
if (config.build.lib) {
// #7188, esbuild adds helpers out of the UMD and IIFE wrappers, and the
// names are minified potentially causing collision with other globals.
// We inject the helpers inside the wrappers.
// e.g. turn:
// <esbuild helpers> (function(){ /*actual content/* })()
// into:
// (function(){ <esbuild helpers> /*actual content/* })()
// Not using regex because it's too hard to rule out performance issues like #8738 #8099 #10900 #14065
// Instead, using plain string index manipulation (indexOf, slice) which is simple and performant
// We use a regex to inject the helpers inside the wrappers.
// We don't need to create a MagicString here because both the helpers and
// the headers don't modify the sourcemap
const esbuildCode = res.code
const contentIndex =
opts.format === 'iife'
? esbuildCode.match(IIFE_BEGIN_RE)?.index || 0
: opts.format === 'umd'
? esbuildCode.indexOf(`(function(`) // same for minified or not
: 0
if (contentIndex > 0) {
const esbuildHelpers = esbuildCode.slice(0, contentIndex)
res.code = esbuildCode
.slice(contentIndex)
.replace(`"use strict";`, `"use strict";` + esbuildHelpers)
const injectHelpers =
opts.format === 'umd'
? INJECT_HELPERS_UMD_RE
: opts.format === 'iife'
? INJECT_HELPERS_IIFE_RE
: undefined
if (injectHelpers) {
res.code = res.code.replace(
injectHelpers,
(_, helpers, header) => header + helpers,
)
}
}

return res
},
}
Expand Down
4 changes: 1 addition & 3 deletions playground/lib/__tests__/lib.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ describe.runIf(isBuild)('build', () => {
'dist/nominify/my-lib-custom-filename.iife.js',
)
// esbuild helpers are injected inside of the IIFE wrapper
// esbuild has a bug that injects some statements before `"use strict"`: https://github.com/evanw/esbuild/issues/3322
// remove the `.*?` part once it's fixed
expect(code).toMatch(/^var MyLib=function\(\)\{.*?"use strict";/)
expect(code).toMatch(/^var MyLib=function\(\)\{"use strict";/)
expect(noMinifyCode).toMatch(
/^var MyLib\s*=\s*function\(\)\s*\{.*?"use strict";/s,
)
Expand Down
3 changes: 0 additions & 3 deletions playground/lib/src/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,3 @@ export default function myLib(sel) {
// make sure umd helper has been moved to the right position
console.log(`amd function(){ "use strict"; }`)
}

// For triggering unhandled global esbuild helpers in previous regex-based implementation for injection
;(function () {})()?.foo
1 change: 0 additions & 1 deletion playground/lib/vite.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ export default defineConfig({
supported: {
// Force esbuild inject helpers to test regex
'object-rest-spread': false,
'optional-chain': false,
},
},
build: {
Expand Down

3 comments on commit 54e1275

@fnlctrl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluwy what was the regression caused by this commit, was there an associated issue?

@bluwy
Copy link
Member Author

@bluwy bluwy commented on 54e1275 Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes #14537

@fnlctrl
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluwy Thank you! I was checking from the release note's link to this commit without realizing it's v4 and not main. I guess I'll upgrade to v5 then😂

Please sign in to comment.