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: improve the rendering of build errors when bundling #6894

Merged
merged 1 commit into from
Oct 10, 2024
Merged
Show file tree
Hide file tree
Changes from all 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/famous-camels-leave.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"wrangler": patch
---

fix: improve the rendering of build errors when bundling
76 changes: 34 additions & 42 deletions packages/wrangler/src/__tests__/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8940,22 +8940,20 @@ addEventListener('fetch', event => {});`
);

await expect(
runWrangler("deploy index.js --dry-run").catch(
(e) =>
normalizeString(
esbuild
.formatMessagesSync(e?.errors ?? [], { kind: "error" })
.join()
.trim()
).split("This error came from the")[0]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need for this "test hack" any more.

runWrangler("deploy index.js --dry-run").catch((e) =>
normalizeString(
esbuild
.formatMessagesSync(e?.errors ?? [], { kind: "error" })
.join()
.trim()
)
)
).resolves.toMatchInlineSnapshot(`
"X [ERROR] Unexpected external import of \\"cloudflare:sockets\\". Imports are not valid in a Service Worker format Worker.
Did you mean to create a Module Worker?
"X [ERROR] Unexpected external import of \\"cloudflare:sockets\\".
Your worker has no default export, which means it is assumed to be a Service Worker format Worker.
Did you mean to create a ES Module format Worker?
If so, try adding \`export default { ... }\` in your entry-point.
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/. [plugin Cloudflare internal imports plugin]

"
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/. [plugin cloudflare-internal-imports]"
`);
});

Expand All @@ -8973,24 +8971,20 @@ addEventListener('fetch', event => {});`
);

await expect(
runWrangler("deploy index.js --dry-run").catch(
(e) =>
normalizeString(
esbuild
.formatMessagesSync(e?.errors ?? [], { kind: "error" })
.join()
.trim()
).split("This error came from the")[0]
runWrangler("deploy index.js --dry-run").catch((e) =>
normalizeString(
esbuild
.formatMessagesSync(e?.errors ?? [], { kind: "error" })
.join()
.trim()
)
)
).resolves.toMatchInlineSnapshot(`
"X [ERROR]
Unexpected external import of \\"node:stream\\". Imports are not valid in a Service Worker format Worker.
Did you mean to create a Module Worker?
If so, try adding \`export default { ... }\` in your entry-point.
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/.
[plugin nodejs_compat imports plugin]

"
"X [ERROR] Unexpected external import of \\"node:stream\\".
Your worker has no default export, which means it is assumed to be a Service Worker format Worker.
Did you mean to create a ES Module format Worker?
If so, try adding \`export default { ... }\` in your entry-point.
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/. [plugin nodejs_compat-imports]"
`);
});

Expand All @@ -9009,22 +9003,20 @@ addEventListener('fetch', event => {});`
);

await expect(
runWrangler("deploy index.js --dry-run").catch(
(e) =>
normalizeString(
esbuild
.formatMessagesSync(e?.errors ?? [], { kind: "error" })
.join()
.trim()
).split("This error came from the")[0]
runWrangler("deploy index.js --dry-run").catch((e) =>
normalizeString(
esbuild
.formatMessagesSync(e?.errors ?? [], { kind: "error" })
.join()
.trim()
)
)
).resolves.toMatchInlineSnapshot(`
"X [ERROR] Unexpected external import of \\"node:stream\\" and \\"node:timers/promises\\". Imports are not valid in a Service Worker format Worker.
Did you mean to create a Module Worker?
"X [ERROR] Unexpected external import of \\"node:stream\\" and \\"node:timers/promises\\".
Your worker has no default export, which means it is assumed to be a Service Worker format Worker.
Did you mean to create a ES Module format Worker?
If so, try adding \`export default { ... }\` in your entry-point.
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/. [plugin unenv-cloudflare]

"
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/. [plugin hybrid-nodejs_compat]"
`);
});
});
Expand Down
14 changes: 14 additions & 0 deletions packages/wrangler/src/deployment-bundle/build-failures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,17 @@ export function isBuildFailure(err: unknown): err is esbuild.BuildFailure {
"warnings" in err
);
}

/**
* Returns true if the error has a cause that is like an esbuild BuildFailure object.
*/
export function isBuildFailureFromCause(
err: unknown
): err is { cause: esbuild.BuildFailure } {
return (
typeof err === "object" &&
err !== null &&
"cause" in err &&
isBuildFailure(err.cause)
);
}
16 changes: 15 additions & 1 deletion packages/wrangler/src/deployment-bundle/bundle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,11 @@ export async function bundleWorker(
await ctx.watch();
result = await initialBuildResultPromise;
if (result.errors.length > 0) {
throw new UserError("Failed to build");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were just swallowing the errors and warnings here and relying upon them being logged "directly" to stdout by another instance of esbuild that is doing the format sniffing. See below.

throw new BuildFailure(
"Initial build failed.",
result.errors,
result.warnings
);
}

stop = async function () {
Expand Down Expand Up @@ -536,3 +540,13 @@ export async function bundleWorker(
},
};
}

class BuildFailure extends Error {
constructor(
message: string,
readonly errors: esbuild.Message[],
readonly warnings: esbuild.Message[]
) {
super(message);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export function esbuildAliasExternalPlugin(
aliases: Record<string, string>
): Plugin {
return {
name: "external alias imports plugin",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When writing to the logs esbuild already prefixes these names with plugin: so I removed all the " plugin" postfixes and made them all bit more consistent.

name: "external-alias-imports",
setup(build) {
build.onResolve({ filter: /.*/g }, (args) => {
// If it's the entrypoint, let it be as is
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type { Plugin } from "esbuild";
* An esbuild plugin that will mark `node:async_hooks` imports as external.
*/
export const asyncLocalStoragePlugin: Plugin = {
name: "Mark async local storage imports as external plugin",
name: "async-local-storage-imports",
setup(pluginBuild) {
pluginBuild.onResolve({ filter: /^node:async_hooks(\/|$)/ }, () => {
return { external: true };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type { Plugin } from "esbuild";
* An esbuild plugin that will mark any `cloudflare:...` imports as external.
*/
export const cloudflareInternalPlugin: Plugin = {
name: "Cloudflare internal imports plugin",
name: "cloudflare-internal-imports",
setup(pluginBuild) {
const paths = new Set();
pluginBuild.onStart(() => paths.clear());
Expand All @@ -23,14 +23,19 @@ export const cloudflareInternalPlugin: Plugin = {
.map((p) => `"${p}"`)
.sort()
);
throw new Error(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that throwing an error causes esbuild to think that something bad went wrong in the plugin and dumps the whole stack trace to the plugin out.
Returning an object containing errors for user errors is cleaner.

dedent`
Unexpected external import of ${pathList}. Imports are not valid in a Service Worker format Worker.
Did you mean to create a Module Worker?
If so, try adding \`export default { ... }\` in your entry-point.
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/.
`
);
return {
errors: [
{
text: dedent`
Unexpected external import of ${pathList}.
Your worker has no default export, which means it is assumed to be a Service Worker format Worker.
Did you mean to create a ES Module format Worker?
If so, try adding \`export default { ... }\` in your entry-point.
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/.
`,
},
],
};
}
});
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export function configProviderPlugin(
config: Record<string, Record<string, unknown>>
): Plugin {
return {
name: "middleware config provider plugin",
name: "middleware-config-provider",
setup(build) {
build.onResolve({ filter: /^config:/ }, (args) => ({
path: args.path,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const REQUIRED_UNENV_ALIAS_NAMESPACE = "required-unenv-alias";
export const nodejsHybridPlugin: () => Plugin = () => {
const { alias, inject, external } = env(nodeless, cloudflare);
return {
name: "unenv-cloudflare",
name: "hybrid-nodejs_compat",
setup(build) {
errorOnServiceWorkerFormat(build);
handleRequireCallsToNodeJSBuiltins(build);
Expand Down Expand Up @@ -43,14 +43,19 @@ function errorOnServiceWorkerFormat(build: PluginBuild) {
.map((p) => `"${p}"`)
.sort()
);
throw new Error(
dedent`
Unexpected external import of ${pathList}. Imports are not valid in a Service Worker format Worker.
Did you mean to create a Module Worker?
If so, try adding \`export default { ... }\` in your entry-point.
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/.
`
);
return {
errors: [
{
text: dedent`
Unexpected external import of ${pathList}.
Your worker has no default export, which means it is assumed to be a Service Worker format Worker.
Did you mean to create a ES Module format Worker?
If so, try adding \`export default { ... }\` in your entry-point.
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/.
`,
},
],
};
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,27 @@ export function logBuildOutput(
onStart?.();
});
build.onEnd(async ({ errors, warnings }) => {
if (errors.length > 0) {
if (nodejsCompatMode !== "legacy") {
rewriteNodeCompatBuildFailure(errors, nodejsCompatMode);
}
logBuildFailure(errors, warnings);
return;
}

if (warnings.length > 0) {
logBuildWarnings(warnings);
}

if (!bundled) {
// First bundle, no need to update bundle
// First bundle, no need to update bundle or log errors
bundled = true;

// But we still want to log warnings as these are not repeated in first-time build failures
if (warnings.length > 0) {
logBuildWarnings(warnings);
}
} else {
if (errors.length > 0) {
if (nodejsCompatMode !== "legacy") {
rewriteNodeCompatBuildFailure(errors, nodejsCompatMode);
}
logBuildFailure(errors, warnings);
return;
}

if (warnings.length > 0) {
logBuildWarnings(warnings);
}

await updateBundle?.();
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import type { Plugin } from "esbuild";
export const nodejsCompatPlugin: (silenceWarnings: boolean) => Plugin = (
silenceWarnings
) => ({
name: "nodejs_compat imports plugin",
name: "nodejs_compat-imports",
setup(pluginBuild) {
// Infinite loop detection
const seen = new Set<string>();
Expand Down Expand Up @@ -71,20 +71,19 @@ export const nodejsCompatPlugin: (silenceWarnings: boolean) => Plugin = (
.map((p) => `"${p}"`)
.sort()
);
throw new Error(`
Unexpected external import of ${paths}. Imports are not valid in a Service Worker format Worker.
Did you mean to create a Module Worker?
If so, try adding \`export default { ... }\` in your entry-point.
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/.
`);
const errors = Array.from(warnedPackaged.entries()).map(
([path, importers]) =>
`Unexpected import "${path}" which is not valid in a Service Worker format Worker. Are you missing \`export default { ... }\` from your Worker?\n` +
"Imported from:\n" +
toList(importers, pluginBuild.initialOptions.absWorkingDir) +
"\n"
);
throw new Error(errors.join(""));
return {
errors: [
{
text: dedent`
Unexpected external import of ${paths}.
Your worker has no default export, which means it is assumed to be a Service Worker format Worker.
Did you mean to create a ES Module format Worker?
If so, try adding \`export default { ... }\` in your entry-point.
See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/.
`,
},
],
};
}
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type { Plugin } from "esbuild";
* An esbuild plugin that will export URL from the url polyfill
*/
export const standardURLPlugin: () => Plugin = () => ({
name: "standard URL plugin",
name: "standard-URL",
setup(pluginBuild) {
pluginBuild.onResolve({ filter: /^node:url$|^url$/ }, ({ importer }) => {
if (importer === "standard-url-plugin") {
Expand Down
9 changes: 7 additions & 2 deletions packages/wrangler/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import { d1 } from "./d1";
import { deleteHandler, deleteOptions } from "./delete";
import { deployHandler, deployOptions } from "./deploy";
import { isAuthenticationError } from "./deploy/deploy";
import { isBuildFailure } from "./deployment-bundle/build-failures";
import {
isBuildFailure,
isBuildFailureFromCause,
} from "./deployment-bundle/build-failures";
import {
commonDeploymentCMDSetup,
deployments,
Expand Down Expand Up @@ -904,7 +907,9 @@ export async function main(argv: string[]): Promise<void> {
} else if (isBuildFailure(e)) {
mayReport = false;
logBuildFailure(e.errors, e.warnings);
logger.error(e.message);
} else if (isBuildFailureFromCause(e)) {
mayReport = false;
logBuildFailure(e.cause.errors, e.cause.warnings);
} else {
let loggableException = e;
if (
Expand Down
10 changes: 7 additions & 3 deletions packages/wrangler/src/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,13 @@ export function logBuildWarnings(warnings: Message[]) {
* style esbuild would.
*/
export function logBuildFailure(errors: Message[], warnings: Message[]) {
const logs = formatMessagesSync(errors, { kind: "error", color: true });
for (const log of logs) {
logger.console("error", log);
if (errors.length > 0) {
const logs = formatMessagesSync(errors, { kind: "error", color: true });
const errorStr = errors.length > 1 ? "errors" : "error";
logger.error(
`Build failed with ${errors.length} ${errorStr}:\n` + logs.join("\n")
);
}

logBuildWarnings(warnings);
}
Loading