Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: usage of wasm in an appDir page file using the edge runtime #41689

Merged
merged 5 commits into from Nov 3, 2022
Merged

fix: usage of wasm in an appDir page file using the edge runtime #41689

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d66819
fix usage of wasm in an appDir page file using the edge runtime
Oct 23, 2022
638cfcb
Merge branch 'canary' into brk.fix/app-dir-edge-load-wasm
balazsorban44 Oct 25, 2022
ede03bc
Merge branch 'canary' into brk.fix/app-dir-edge-load-wasm
BRKalow Oct 26, 2022
6221673
updates test per review
Oct 26, 2022
63c70aa
Merge branch 'canary' into brk.fix/app-dir-edge-load-wasm
ijjk Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/next/build/utils.ts
View file
Open in desktop
Expand Up @@ -52,6 +52,7 @@ import {
loadRequireHook,
overrideBuiltInReactPackages,
} from './webpack/require-hook'
import { AssetBinding } from './webpack/loaders/get-module-build-info'

loadRequireHook()
if (process.env.NEXT_PREBUNDLED_REACT) {
Expand Down Expand Up @@ -1259,7 +1260,13 @@ export async function isPageStatic({
const runtime = await getRuntimeContext({
paths: edgeInfo.files.map((file: string) => path.join(distDir, file)),
env: edgeInfo.env,
edgeFunctionEntry: edgeInfo,
edgeFunctionEntry: {
...edgeInfo,
wasm: (edgeInfo.wasm ?? []).map((binding: AssetBinding) => ({
...binding,
filePath: path.join(distDir, binding.filePath),
})),
},
Comment on lines +1263 to +1269
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch :clapping-inclusive:

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need to do the same for blob assets? (i.e. fetch(new URL("./file.png", import.meta.url)))

anyway that can be in a new PR :open_source_parrot:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question 🤔 yeah I can take a look and add a test case as a follow-up!

name: edgeInfo.name,
useCache: true,
distDir,
Expand Down
Binary file added test/production/app-dir-edge-runtime-with-wasm/add.wasm
View file
Open in desktop
Binary file not shown.
62 changes: 62 additions & 0 deletions test/production/app-dir-edge-runtime-with-wasm/index.test.ts
View file
Open in desktop
@@ -0,0 +1,62 @@
import path from 'path'
import { createNext, FileRef } from 'e2e-utils'
import { NextInstance } from 'test/lib/next-modes/base'
import { renderViaHTTP } from 'next-test-utils'

const files = {
'app/layout.tsx': `
export default function AppLayout({ children }) {
return (
<html>
<head>
<title>WASM Import</title>
</head>
<body>
{children}
</body>
</html>
)
}
`,
'app/page.tsx': `
// @ts-expect-error
import wasm from '../wasm/add.wasm?module'

console.log(wasm)

export default function Page() {
return "index page"
}

export const runtime = "experimental-edge"
`,
'wasm/add.wasm': new FileRef(path.join(__dirname, 'add.wasm')),
}

describe('app-dir edge runtime with wasm', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it would be preferred to have all of the app-dir related tests in test/e2e/app-dir, let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's good. We can always merge tests. Extracting is harder. 🙏

let next: NextInstance

beforeAll(async () => {
next = await createNext({
files,
dependencies: {
react: 'experimental',
'react-dom': 'experimental',
typescript: 'latest',
'@types/react': 'latest',
'@types/node': 'latest',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider writing the tests in JavaScript and not in TypeScript, so we can remove these dependencies. wdyt? :eyeses:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea, updated!

},
nextConfig: {
experimental: {
appDir: true,
},
},
})
})
afterAll(() => next.destroy())

it('should have built', async () => {
const html = await renderViaHTTP(next.url, '/')
expect(html).toContain('index page')
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we execute the wasm? & see that it actually loads the Wasm file correctly in runtime? :pray_parrot:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated! 😃

})