Skip to content

[fix] ensure completed_build is reset #5541

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

Merged
merged 3 commits into from
Jul 15, 2022
Merged

[fix] ensure completed_build is reset #5541

merged 3 commits into from
Jul 15, 2022

Conversation

benmccann
Copy link
Member

I missed this while reviewing #5536 the first time around

@changeset-bot
Copy link

changeset-bot bot commented Jul 15, 2022

🦋 Changeset detected

Latest commit: 2177d34

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

@@ -176,6 +176,7 @@ function kit() {
vite_config_env = config_env;
svelte_config = await load_config();
is_build = config_env.command === 'build';
completed_build = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Dominik mentioned build --watch, I think it should be in buildStart for that to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@benmccann benmccann Jul 15, 2022

Choose a reason for hiding this comment

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

I don't understand. When the server restarts in that stackblitz, I see config called. @dominikg apparently mentioned something and I see @bluwy gave a thumbs up as well. What am I missing?

To me, config is a better place to put it because it happens sooner and we're already setting / resetting other variables there. If we set everything at the very beginning it saves us from having to worry about whether we've done it before the variable is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

When the server restarts in that stackblitz, I see config called.

The server doesn't restart in build --watch, config is only called when the plugin is loaded initially and we're already setting it to false where it's declared.

Let's say a build succeeds, the user then makes a change to some page which causes a build error, but config isn't called again so the flag isn't reset and it tries to go ahead with adapting in that case even though it shouldn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see. The stackblitz isn't using build --watch 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

Oop, yeah mb, couldn't get it working according to their docs for startCommand, just realized I coulda just changed the dev script to build --watch.

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.

3 participants