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(nextjs): Fix HMR by inserting new entrypoints at the end #9267

Merged
merged 4 commits into from
Oct 17, 2023

Conversation

lforst
Copy link
Member

@lforst lforst commented Oct 16, 2023

Fixes #9067

The order in which we insert the SDK into the webpack entrypoints seems to matter in dev mode. Maybe the dev init script is somehow referenced by index. 🤷‍♂️

There is a slight nuanced consideration here in which the SDK may now be initialized too late to catch any errors generated by Next.js which we insert at the end only in dev mode.

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

So many edge cases... 🥲

Comment on lines 588 to 590
newEntryPoint = isDevMode ? [currentEntryPoint, ...filesToInsert] : [...filesToInsert, currentEntryPoint];
} else if (Array.isArray(currentEntryPoint)) {
newEntryPoint = [...filepaths, ...currentEntryPoint];
newEntryPoint = isDevMode ? [...currentEntryPoint, ...filesToInsert] : [...filesToInsert, ...currentEntryPoint];
Copy link
Member

Choose a reason for hiding this comment

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

l: took me a while to understand the cases and object spreads here. Not sure how much we can simplify this but do you think it'd help to use our arrayify utility to basically remove the typeof checks? Although then again I'm not sure what types currentEntryPoint could be of. Feel free to disregard!

): void {
// BIG FAT NOTE: Order of insertion seems to matter here. If we insert the new files before the `currentEntrypoint`s, the Next.js dev server breaks.
Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Much cleaner!

@lforst lforst merged commit d46bc08 into develop Oct 17, 2023
52 checks passed
@lforst lforst deleted the lforst-fix-9067 branch October 17, 2023 07:52
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.

withSentryConfig breaks auto-reload for next.js dev
3 participants