Skip to content

fix(middleware): Support wasm in bundled middleware #802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 28, 2025
Merged

fix(middleware): Support wasm in bundled middleware #802

merged 3 commits into from
Mar 28, 2025

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Mar 27, 2025

Related to opennextjs/opennextjs-cloudflare#471

2 fixes in this PR:

  • For bundled middleware, we do copy the source file (in createServerBundle but we also need to copy .wasm files
  • The second fix is specific for the edge, the file name is used in Next middleware source, then if we re-use the same name to import the same file, ESBulid will append a 2 and Next can not reference this import.

Copy link

changeset-bot bot commented Mar 27, 2025

🦋 Changeset detected

Latest commit: c2dfbd6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@opennextjs/aws Patch
app-pages-router Patch
app-router Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Mar 27, 2025

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 26.15% 2358 / 9014
🔵 Statements 26.15% 2358 / 9014
🔵 Functions 52.71% 126 / 239
🔵 Branches 73.15% 586 / 801
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/open-next/src/build/createServerBundle.ts 0% 0% 0% 0% 1-375
packages/open-next/src/build/edge/createEdgeBundle.ts 0% 0% 0% 0% 1-249
packages/open-next/src/plugins/edge.ts 0% 0% 0% 0% 1-218
Generated in workflow #1114 for commit c2dfbd6 by the Vitest Coverage Report Action

Copy link

pkg-pr-new bot commented Mar 27, 2025

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/aws@802

commit: c2dfbd6

path.join(outPackagePath, "middleware.mjs"),
);

const middlewareManifest = loadMiddlewareManifest(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should create a generic function for this and use it both here and there

//Copy wasm files
const wasmFiles = middlewareInfo.wasm;
mkdirSync(path.join(outputDir, "wasm"), { recursive: true });
for (const wasmFile of wasmFiles) {
fs.copyFileSync(
path.join(buildOutputDotNextDir, wasmFile.filePath),
path.join(outputDir, `wasm/${wasmFile.name}.wasm`),
);
}
// Copy assets
const assets = middlewareInfo.assets;
mkdirSync(path.join(outputDir, "assets"), { recursive: true });
for (const asset of assets) {
fs.copyFileSync(
path.join(buildOutputDotNextDir, asset.filePath),
path.join(outputDir, `assets/${asset.name}`),
);
}

We also need to copy the assets

? `import ${file.name} from './wasm/${file.name}.wasm';`
? // Decorate the name to avoid name collisions
`import __OpenNextWasm${i} from './wasm/${file.name}.wasm';
globalThis.${file.name} = __OpenNextWasm${i}`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm probably missing something here, but if there is collision, wouldn't this also cause some issue with the global ?

Only things that use this (without being a separate function) should be the middleware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.next/server/src/middleware.js would have something like

683: (e) => {
  e.exports = wasm_ea010aa52414c7de383495546f15cb7d49b055fc;
}

If the code here is

  import wasm_ea010aa52414c7de383495546f15cb7d49b055fc from '...';

Then ESBuild will change that to wasm_ea010aa52414c7de383495546f15cb7d49b055fc2 (appending a 2 at the end).

So we want the "collision" here.

@@ -159,15 +162,26 @@ async function generateBundle(
if (!config.middleware?.external) {
fs.copyFileSync(
path.join(options.buildDir, "middleware.mjs"),
path.join(outputPath, packagePath, "middleware.mjs"),
path.join(outPackagePath, "middleware.mjs"),
Copy link
Contributor

Choose a reason for hiding this comment

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

All this copy logic should also be in a function on this file so that everything middleware related is there https://github.com/opennextjs/opennextjs-aws/blob/main/packages/open-next/src/build/createMiddleware.ts.

Pretty sure we need to copy the wasm/asset files for the external middleware as well

@vicb vicb requested a review from conico974 March 28, 2025 13:55
Copy link
Contributor

@conico974 conico974 left a comment

Choose a reason for hiding this comment

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

LGTM Thanks

@vicb
Copy link
Contributor Author

vicb commented Mar 28, 2025

Thanks for the review Nico!

@vicb vicb merged commit 8392e22 into main Mar 28, 2025
3 checks passed
@vicb vicb deleted the vb-branch-3 branch March 28, 2025 14:57
@github-actions github-actions bot mentioned this pull request Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants