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.

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

fix: patch @vercel/og usage to use the edge runtime version #283

Merged
merged 13 commits into from
Jan 27, 2025
Merged

Conversation

james-elicx
Copy link
Collaborator

PR is based off of #282 and should not be merged before that one.

This PR does the following:

  • Changes the function for getting rule edits to also enable returning matches.
  • Patches the import of index.node.js to index.edge.js.
  • Patches the fallback font fetch to use an import.
  • Renames the fallback font to be suffixed with .bin.
  • Copies the edge version to the OpenNext node_modules.
  • Unit tests + e2e test.

Might be worth noting that the edge runtime version is only available in the node_modules location referenced by the traced files in .next/server, and not .next/standalone. So we need to use that output to get the edge file and work our way back up to node_modules, and bring that over the right place for the standalone build in our output.

Not sure if this approach is entirely what you had in mind.

@james-elicx james-elicx linked an issue Jan 26, 2025 that may be closed by this pull request
Copy link

changeset-bot bot commented Jan 26, 2025

🦋 Changeset detected

Latest commit: 7478813

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

This PR includes changesets to release 1 package
Name Type
@opennextjs/cloudflare 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

pkg-pr-new bot commented Jan 26, 2025

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/cloudflare@283

commit: 7478813

@james-elicx james-elicx marked this pull request as ready for review January 26, 2025 21:26
@vicb
Copy link
Contributor

vicb commented Jan 27, 2025

Might be worth noting that the edge runtime version is only available in the node_modules location referenced by the traced files in .next/server, and not .next/standalone. So we need to use that output to get the edge file and work our way back up to node_modules, and bring that over the right place for the standalone build in our output.

Yep, I created #285 to track this

Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks!

.github/workflows/checks.yml Show resolved Hide resolved
examples/api/e2e/base.spec.ts Show resolved Hide resolved
packages/cloudflare/src/cli/build/patches/ast/vercel-og.ts Outdated Show resolved Hide resolved
packages/cloudflare/src/cli/build/patches/ast/vercel-og.ts Outdated Show resolved Hide resolved
kind: variable_declarator
has:
kind: identifier
regex: ^fallbackFont$
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe it would be safer to detect the fetch(font_name) in case the variable name changes - but let's merge this, we can refine later if needed.

@@ -33,7 +33,8 @@ export async function bundleServer(buildOpts: BuildOptions): Promise<void> {
console.log(`\x1b[35m⚙️ Bundling the OpenNext server...\n\x1b[0m`);

patches.patchWranglerDeps(buildOpts);
patches.updateWebpackChunksFile(buildOpts);
await patches.updateWebpackChunksFile(buildOpts);
await patches.patchVercelOgLibrary(buildOpts);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to be able to move the changes to updateWorkerBundledCode()
It would probably require adding externals to the ESBuild config.
With that we could parse() (ast-grep) the code once and apply multiple edits.

(This again could be handled in a follow-up PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we can do it there as this is patching a specific route's file whereas that function is patching handler.mjs, and the code for each of the routes doesn't appear to be bundled into the handler.mjs

Copy link
Contributor

Choose a reason for hiding this comment

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

the code for each of the routes doesn't appear to be bundled into the handler.mjs

yet :)

As I mentioned, we should move patches that add dependencies (require) from after the bundle to before/during the bundling.

I do have an ast-grep patch solving the route issue. I need to rebase and submit the PR.

@vicb vicb merged commit d707bd3 into paths Jan 27, 2025
7 checks passed
@vicb vicb deleted the james/og branch January 27, 2025 09:21
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.

[BUG] Unable use @vercel/og with OpenNext
2 participants