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

Log route module errors prior to reloading the page #8932

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Feb 28, 2024

Closes #8791, #8915

Copy link

changeset-bot bot commented Feb 28, 2024

🦋 Changeset detected

Latest commit: 045fae6

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

This PR includes changesets to release 16 packages
Name Type
@remix-run/react Patch
@remix-run/testing Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/serve Patch
@remix-run/server-runtime 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

console.error(
`Error loading route module \`${route.module}\`, reloading page...`
);
console.error(error);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've never logged this error before since it's swallowed on the reload and (I think) it was much harder to run into this for import issues in dev when using esbuild. But using vite during dev, you don't find out about these module errors until runtime, so we can log them and they're available via Preserve log

I'm curious through if they could be surfaced through Vite's overlay? @pcattori @markdalgleish

throw error;
}

window.location.reload();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also consider enhancing this behavior in the future too. Reloading the current page isn't necessarily what we want all the time. There's 2 usages of this method:

  • prefetching - in that case there's no real reason to hard reload since the user may not click on the link, so we could just let the prefetch no-op
  • navigating - ideally for the most seamless UX we'd hard-navigate to the next url instead of reloading the current URL in response to a clicked link. The issue here is that currently route.lazy (which calls this) isn't aware of the next location so we'd have to plumb that through in RR if we wanted to smooth out that flow.

Copy link

Choose a reason for hiding this comment

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

It has bugged me many times that the current page gets reloaded instead of the target URL.

@brophdawg11 brophdawg11 merged commit 5bccd1e into dev Jul 12, 2024
5 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/log-module-error branch July 12, 2024 15:46
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Jul 12, 2024
Copy link
Contributor

🤖 Hello there,

We just published version 2.10.3-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.10.3 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redirects silently fail to a route that has an import error <Link> tag reloads page instead of navigating
4 participants