fix: avoid concealing build errors #5520

gtm-nayan · 2022-07-14T07:31:31Z

closes #5413

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests

Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

changeset-bot · 2022-07-14T07:31:34Z

🦋 Changeset detected

Latest commit: 681d785

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

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	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

packages/kit/src/vite/index.js

gtm-nayan · 2022-07-14T14:13:31Z

Not yet, https://discord.com/channels/457912077277855764/571775594002513921/997058576075997214

Rich-Harris · 2022-07-14T19:15:30Z

This feels like quite a bit of complexity. IIRC the reason we're not doing the adapt step inside writeBundle is that calling process.exit(0) in that hook risks interfering with other plugins. What if we just followed through on #5306 — could we avoid all this?

benmccann · 2022-07-14T19:40:42Z

I had wondered as well and #5306 would certainly be nice to fix given the number of issues it has caused, but I think the answer is that we still need this because what would it mean to run the adapter on a failed build? I talked with patak that we need additional hooks at the end of the Vite build, so I think that may be part of the answer long-term as well

Rich-Harris · 2022-07-14T20:08:22Z

If adapting was part of writeBundle then it wouldn't run on a failed build. we wouldn't get that far into the function

benmccann · 2022-07-14T20:14:00Z

Yes, but it would break Vite plugins that output anything during writeBundle as the adapter could no longer utilize their output

Rich-Harris · 2022-07-14T20:47:31Z

Ah, I see. What does that mean in practice though? Are there any adapters that depend on the output of a Vite plugin? Even if so, it feels like all that would need to happen is that the SvelteKit plugin goes after any plugins that generate output the adapter depends on.

benmccann · 2022-07-14T20:56:36Z

Ah, I see. What does that mean in practice though? Are there any adapters that depend on the output of a Vite plugin?

It's not really specific to any adapter. E.g. vite-plugin-pwa generates files during writeBundle, but they weren't making them from .svelte-kit to build or wherever until we moved the adapt phase to happen later

Even if so, it feels like all that would need to happen is that the SvelteKit plugin goes after any plugins that generate output the adapter depends on.

Unfortunately not because all writeBundle hooks run in parallel

Rich-Harris

given that we're not using the actual values of build_error or write_bundle_error anywhere, can we just track it as a boolean? suggestions inline

Rich-Harris · 2022-07-14T21:15:08Z

packages/kit/src/vite/index.js

+	/** @type {Error | undefined} */
+	let build_error;
+
+	/** @type {Error | undefined} */
+	let write_bundle_error;


Suggested change

/** @type {Error | undefined} */

let build_error;

/** @type {Error | undefined} */

let write_bundle_error;

let errored = false;

Rich-Harris · 2022-07-14T21:15:29Z

packages/kit/src/vite/index.js

@@ -247,6 +253,9 @@ function kit() {
 		 * Clears the output directories.
 		 */
 		buildStart() {
+			// Reset errors in watch mode
+			build_error = write_bundle_error = undefined;


Suggested change

build_error = write_bundle_error = undefined;

errored = false;

Rich-Harris · 2022-07-14T21:15:51Z

packages/kit/src/vite/index.js

@@ -256,6 +265,10 @@ function kit() {
 			}
 		},

+		buildEnd(err) {
+			build_error = err;


Suggested change

build_error = err;

errored = !!err;

Rich-Harris · 2022-07-14T21:16:19Z

packages/kit/src/vite/index.js

-
-				await build_service_worker(options, prerendered, client.vite_manifest);
+			} catch (error) {
+				throw (write_bundle_error = /** @type {Error} */ (error));


Suggested change

throw (write_bundle_error = /** @type {Error} */ (error));

errored = true;

throw (error);

Rich-Harris · 2022-07-14T21:18:00Z

packages/kit/src/vite/index.js

@@ -354,6 +373,11 @@ function kit() {
 			if (!is_build) {
 				return; // vite calls closeBundle when dev-server restarts, ignore that
 			}
+
+			if (build_error || write_bundle_error) {


Suggested change

if (build_error || write_bundle_error) {

if (errored) {

Rich-Harris · 2022-07-14T21:24:45Z

in fact, could we make it even simpler than my suggestion above? rather than tracking failure, we could track success. presumably writeBundle isn't called if the build fails, so we could get rid of the buildEnd hook, and just set completed_build = true at the end of writeBundle. opening a PR against this branch, one sec

Rich-Harris · 2022-07-14T21:28:37Z

#5536

* loop over values instead with `Object.values` * missed a closing bracket * avoid concealing build errors * fix comment * changeset * Handle errors in writeBundle * inline comments * track success rather than failure * oops, missing piece Co-authored-by: Ignatius Bagus <ignatius.mbs@gmail.com> Co-authored-by: gtm-nayan <gtmnayan@gmail.com>

Rich-Harris · 2022-07-14T23:24:05Z

closing since #5536 was merged — thanks all

vercel bot deployed to Preview – kit July 14, 2022 07:32 View deployment

Rich-Harris reviewed Jul 14, 2022

View reviewed changes

packages/kit/src/vite/index.js Outdated Show resolved Hide resolved

Rich-Harris approved these changes Jul 14, 2022

View reviewed changes

vercel bot deployed to Preview – kit July 14, 2022 14:07 View deployment

bluwy approved these changes Jul 14, 2022

View reviewed changes

vercel bot deployed to Preview – kit-svelte-dev July 14, 2022 14:09 View deployment

gtm-nayan marked this pull request as draft July 14, 2022 14:13

ignatiusmb and others added 7 commits July 14, 2022 22:02

loop over values instead with Object.values

56754ea

missed a closing bracket

0e7bfe3

avoid concealing build errors

6405386

fix comment

2f845d1

changeset

9ac7ca8

Handle errors in writeBundle

7c2e838

inline comments

681d785

gtm-nayan force-pushed the missing-imports branch from cb9df3b to 681d785 Compare July 14, 2022 16:17

gtm-nayan marked this pull request as ready for review July 14, 2022 16:18

vercel bot deployed to Preview – kit July 14, 2022 16:18 View deployment

gtm-nayan requested review from Rich-Harris and bluwy July 14, 2022 16:20

benmccann approved these changes Jul 14, 2022

View reviewed changes

bluwy approved these changes Jul 14, 2022

View reviewed changes

Rich-Harris reviewed Jul 14, 2022

View reviewed changes

Rich-Harris mentioned this pull request Jul 14, 2022

Alternative to #5520 #5536

Merged

Rich-Harris closed this Jul 14, 2022

gtm-nayan deleted the missing-imports branch July 16, 2022 06:55

	build_error = write_bundle_error = undefined;
	errored = false;

	throw (write_bundle_error = /** @type {Error} */ (error));
	errored = true;
	throw (error);

Uh oh!

fix: avoid concealing build errors #5520

fix: avoid concealing build errors #5520

Uh oh!

Conversation

gtm-nayan commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

Tests

Changesets

Uh oh!

changeset-bot bot commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Uh oh!

gtm-nayan commented Jul 14, 2022

Uh oh!

Rich-Harris commented Jul 14, 2022

Uh oh!

benmccann commented Jul 14, 2022

Uh oh!

Rich-Harris commented Jul 14, 2022

Uh oh!

benmccann commented Jul 14, 2022

Uh oh!

Rich-Harris commented Jul 14, 2022

Uh oh!

benmccann commented Jul 14, 2022

Uh oh!

Rich-Harris left a comment

Choose a reason for hiding this comment

Uh oh!

Rich-Harris Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

Rich-Harris Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

Rich-Harris Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

Rich-Harris Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

Rich-Harris Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

Rich-Harris commented Jul 14, 2022

Uh oh!

Rich-Harris commented Jul 14, 2022

Uh oh!

Rich-Harris commented Jul 14, 2022

Uh oh!

Uh oh!

gtm-nayan commented Jul 14, 2022 •

edited

Loading

changeset-bot bot commented Jul 14, 2022 •

edited

Loading