-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[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
Conversation
🦋 Changeset detectedLatest commit: 2177d34 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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
Outdated
@@ -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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
😆
There was a problem hiding this comment.
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
.
I missed this while reviewing #5536 the first time around