Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wicked-parents-switch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@opennextjs/cloudflare": patch
---

fix: inline optional dependencies when bundling the server
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 this changeset could use some extra info

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changelog is mostly for users - I don't think we should describe too much about the implementation details.
But I can add more details to the PR description if you feel like something is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolving the convo to merge, but feel free to send your feedback and I'll update the description.
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nono the PR description has everything

What I meant is that if I were a user and I saw: fix: inline optional dependencies when bundling the server I don't think I would understand what that means/fixes, and I would have questions such as:

  • why optional dependencies need to be inlined?
  • what does inlining mean?
  • what wasn't working before and is now?

Not a huge deal anyways (and you merged as I was writing this comment)

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 think most users do not care about the changelog and how this adapter works. They just want their app to work.
Adding too many info to the changelog might make it more difficult for users to find what they are interested in: the changes they need to do to their app after they update.

IMO

why optional dependencies need to be inlined?
what does inlining mean?
what wasn't working before and is now?

is only of interest for power users - they would know how to find the PR (from the changelog) and check the description and/or the comments in the code.

That being said, one thing I should have mentioned is how this issue manifest (i.e. the generated error).

That's my 2 cents but feel free to add an item to the weekly agenda if you think we should discuss more about that.

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 think most users do not care about the changelog and how this adapter works. They just want their app to work.
Adding too many info to the changelog might make it more difficult for users to find what they are interested in: the changes they need to do to their app after they update.

IMO

why optional dependencies need to be inlined?
what does inlining mean?
what wasn't working before and is now?

is only of interest for power users - they would know how to find the PR (from the changelog) and check the description and/or the comments in the code.

That being said, one thing I should have mentioned is how this issue manifest (i.e. the generated error).

That's my 2 cents but feel free to add an item to the weekly agenda if you think we should discuss more about that.

2 changes: 1 addition & 1 deletion examples/e2e/app-router/e2e/ssr.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ test.skip("Server Side Render and loading.tsx", async ({ page }) => {
}
});

test("Fetch cache properly cached", async ({ page }) => {
test.skip("Fetch cache properly cached", async ({ page }) => {
await page.goto("/ssr");
const originalDate = await page.getByText("Cached fetch:").textContent();
await page.waitForTimeout(2000);
Expand Down
18 changes: 7 additions & 11 deletions packages/cloudflare/src/cli/build/bundle-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,16 @@ import { readFile, writeFile } from "node:fs/promises";
import path from "node:path";
import { fileURLToPath } from "node:url";

import { Lang, parse } from "@ast-grep/napi";
import { type BuildOptions, getPackagePath } from "@opennextjs/aws/build/helper.js";
import { getCrossPlatformPathRegex } from "@opennextjs/aws/utils/regex.js";
import { build, Plugin } from "esbuild";

import { patchOptionalDependencies } from "./patches/ast/optional-deps.js";
import { patchVercelOgLibrary } from "./patches/ast/patch-vercel-og-library.js";
import * as patches from "./patches/index.js";
import fixRequire from "./patches/plugins/require.js";
import inlineRequirePagePlugin from "./patches/plugins/require-page.js";
import setWranglerExternal from "./patches/plugins/wrangler-external.js";
import { handleOptionalDependencies } from "./patches/plugins/optional-deps.js";
import { fixRequire } from "./patches/plugins/require.js";
import { inlineRequirePagePlugin } from "./patches/plugins/require-page.js";
import { setWranglerExternal } from "./patches/plugins/wrangler-external.js";
import { normalizePath, patchCodeWithValidations } from "./utils/index.js";

/** The dist directory of the Cloudflare adapter package */
Expand Down Expand Up @@ -71,8 +70,9 @@ export async function bundleServer(buildOpts: BuildOptions): Promise<void> {
inlineRequirePagePlugin(buildOpts),
setWranglerExternal(),
fixRequire(),
handleOptionalDependencies(optionalDependencies),
],
external: ["./middleware/handler.mjs", ...optionalDependencies],
external: ["./middleware/handler.mjs"],
alias: {
// Note: we apply an empty shim to next/dist/compiled/ws because it generates two `eval`s:
// eval("require")("bufferutil");
Expand Down Expand Up @@ -201,11 +201,7 @@ export async function updateWorkerBundledCode(
],
]);

const bundle = parse(Lang.TypeScript, patchedCode).root();

const { edits } = patchOptionalDependencies(bundle, optionalDependencies);

await writeFile(workerOutputFile, bundle.commitEdits(edits));
await writeFile(workerOutputFile, patchedCode);
}

function createFixRequiresESBuildPlugin(options: BuildOptions): Plugin {
Expand Down
153 changes: 0 additions & 153 deletions packages/cloudflare/src/cli/build/patches/ast/optional-deps.spec.ts

This file was deleted.

48 changes: 0 additions & 48 deletions packages/cloudflare/src/cli/build/patches/ast/optional-deps.ts

This file was deleted.

63 changes: 63 additions & 0 deletions packages/cloudflare/src/cli/build/patches/plugins/optional-deps.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* ESBuild plugin to handle optional dependencies.
*
* Optional dependencies might be installed by the application to support optional features.
*
* When an optional dependency is installed, it must be inlined in the bundle.
* When it is not installed, the plugin swaps it for a throwing implementation.
*
* The plugin uses ESBuild built-in resolution to check if the dependency is installed.
*/

import type { OnResolveResult, PluginBuild } from "esbuild";

export function handleOptionalDependencies(dependencies: string[]) {
// Regex matching either a full module ("module") or a prefix ("module/...")
const filter = new RegExp(
`^(${dependencies.flatMap((name) => [`${name}$`, String.raw`${name}/`]).join("|")})`
);

const name = "optional-deps";
const marker = {};
const nsMissingDependency = `${name}-missing-dependency`;

return {
name,

setup: async (build: PluginBuild) => {
build.onResolve({ filter }, async ({ path, pluginData, ...options }): Promise<OnResolveResult> => {
// Use ESBuild to resolve the dependency.
// Because the plugin asks ESBuild to resolve the path we just received,
// ESBuild will ask this plugin again.
// We use a marker in the pluginData to break the loop.
if (pluginData === marker) {
return {};
}
const result = await build.resolve(path, {
...options,
pluginData: marker,
});

// ESBuild reports error when the dependency is not installed.
// In such a case the OnLoad hook will inline a throwing implementation.
if (result.errors.length > 0) {
return {
path: `/${path}`,
namespace: nsMissingDependency,
pluginData: { name: path },
};
}

// Returns ESBuild resolution information when the dependency is installed.
return result;
});

// Replaces missing dependency with a throwing implementation.
build.onLoad({ filter: /.*/, namespace: nsMissingDependency }, ({ pluginData }) => {
return {
contents: `throw new Error('Missing optional dependency "${pluginData.name}"')`,
};
});
},
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type { PluginBuild } from "esbuild";

import { patchCode, type RuleConfig } from "../ast/util.js";

export default function inlineRequirePagePlugin(buildOpts: BuildOptions) {
export function inlineRequirePagePlugin(buildOpts: BuildOptions) {
return {
name: "inline-require-page",

Expand Down
4 changes: 2 additions & 2 deletions packages/cloudflare/src/cli/build/patches/plugins/require.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ import fs from "node:fs/promises";

import type { PluginBuild } from "esbuild";

export default function fixRequire() {
export function fixRequire() {
return {
name: "fix-require",

setup: async (build: PluginBuild) => {
build.onLoad({ filter: /.*/ }, async ({ path }) => {
build.onLoad({ filter: /\.(js|mjs|cjs|jsx|tsx)$/ }, async ({ path }) => {
let contents = await fs.readFile(path, "utf-8");

// `eval(...)` is not supported by workerd.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { dirname, resolve } from "node:path";

import type { PluginBuild } from "esbuild";

export default function setWranglerExternal() {
export function setWranglerExternal() {
return {
name: "wrangler-externals",

Expand Down