Enable HMR updates even when critical issues occur #3714
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Ignored Deployments
|
| @@ -284,8 +284,6 @@ function handleSocketMessage(msg: ServerMessage) { | |||
| const hasCriticalIssues = handleIssues(msg); | |||
| const aggregatedMsg = aggregateUpdates(msg, hasCriticalIssues); | |||
|
|
|||
| if (hasCriticalIssues) return; | |||
|
|
|||
There was a problem hiding this comment.
aggregateUpdates specifically adds an empty chunk with update to chunksWithUpdates when hasCriticalIssues = true with the following comment:
add an empty record to make sure we don't call
onBuildOk
Which means it expects the function to continue below and not stop here.
I'm seeing the same issues being reported in both type: "issues" and type: "partial" updates (and these issues are pretty much duplicated for every message received, no matter the chunk), so I also think something's wrong there. There might be a critical error to one chunk, but since it's reported on every single chunk, we disable updating for all of them.
For now this is enough to re-enable HMR updates.
@ForsakenHarmony WDYT?
There was a problem hiding this comment.
The issue is that we might get ourselves into a worse situation if we update and the chunk contains an unparseable item which then requires a hard reload
We need to figure improve what / how we report a critical issue
There was a problem hiding this comment.
I think it should show the error overlay, but queue up the update. Only if you close the error overlay, it will apply the change. Or if another update happens that gets rid of the error.
That what I wrote in the other issue:
Currently HMR updates are ignored when they contain a critical issue.
This has two problems:
- We might miss updates to unrelated modules
- Hiding the error overlay with compile errors will still render HMR non-functional
But we still don't want to trash the whole page with an temporary compile error.
Instead HMR updates with critical errors should be collected and be applied once the error overlay is hidden (or the errors disappear).
Error overlay should re-appear when new critical errors arrive.
There was a problem hiding this comment.
What about the fact that HMR issues are duplicated on every single chunk? There might be chunks where it is unsafe to update because of fatal issues, but other chunks should be fine. Right now, all chunks are considered as "hard error".
@sokra's solution sounds good to me.
There was a problem hiding this comment.
HMR issues are only duplicated if a chunk depends on another chunk that has the original issue
There was a problem hiding this comment.
Got it.
I'll implement @sokra's workaround (i.e. if a user dismisses the overlay, we expect them to be okay with their app breaking).
Can I remove the "add an empty record to make sure we don't call onBuildOk" logic?
There was a problem hiding this comment.
Turns out onBuildOk does nothing at this point, could probably remove that event
we could remove the empty record, and make the calls to onBeforeRefresh & onRefresh dependent on whether the error overlay was dismissed as well
when implementing the workaround, treat the overlay directory as a separate package that we consume (i.e. no imports into another package) to make it easier to merge the overlay with the next.js one later on
|
Benchmark for 3846e5c
Click to view full benchmark
|
# New Features - vercel/turborepo#3695 - vercel/turborepo#3696 # Performance - vercel/turborepo#3738 - vercel/turborepo#3714 - vercel/turborepo#3739 # Fixes - vercel/turborepo#3718 - vercel/turborepo#3618 - vercel/turborepo#3736 # Other Changes - vercel/turborepo#3731 - vercel/turborepo#3733 - vercel/turborepo#3629 - vercel/turborepo#3769 - vercel/turborepo#3707 - vercel/turborepo#3737 - vercel/turborepo#3770 Co-authored-by: JJ Kasper <jj@jjsweb.site>
…3714) This enables HMR updates, even when the update message also contains critical issues. There were some inconsistencies with the logic previously, so I'm not sure whether this fix is fully correct. More on that in PR comments.
…3714) This enables HMR updates, even when the update message also contains critical issues. There were some inconsistencies with the logic previously, so I'm not sure whether this fix is fully correct. More on that in PR comments.
…3714) This enables HMR updates, even when the update message also contains critical issues. There were some inconsistencies with the logic previously, so I'm not sure whether this fix is fully correct. More on that in PR comments.
…3714) This enables HMR updates, even when the update message also contains critical issues. There were some inconsistencies with the logic previously, so I'm not sure whether this fix is fully correct. More on that in PR comments.
…3714) This enables HMR updates, even when the update message also contains critical issues. There were some inconsistencies with the logic previously, so I'm not sure whether this fix is fully correct. More on that in PR comments.
This enables HMR updates, even when the update message also contains critical issues.
There were some inconsistencies with the logic previously, so I'm not sure whether this fix is fully correct. More on that in PR comments.