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 error-sanitization-test #11420

Merged
merged 4 commits into from
Apr 8, 2024
Merged

Conversation

markdalgleish
Copy link
Member

Turns out the logic differed slightly between react-router-dom and @remix-run/react and that Remix serializes stack traces in development mode, so we need to ensure that stack traces are picked up when deserializing errors.

Copy link

changeset-bot bot commented Apr 8, 2024

⚠️ No Changeset found

Latest commit: c51db03

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

// Wipe away the client-side stack trace. Nothing to fill it in with
// because we don't serialize SSR stack traces for security reasons
error.stack = "";
error.stack = val.stack;
Copy link
Contributor

Choose a reason for hiding this comment

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

lol I was super confused for a bit - since we shouldn't be using this "built-in" hydration logic in Remix since we do our own hydration via window.__remixContext. This is used to automatically hydrate from StaticRouterProvider's window.__staticRouterHydrationData - but we specifically pass hydrate=false in the Remix SSR use case.

Turns out we're just using the wrong implementation since we brought the Remix code over. There's a dup version of this method in ssr/errors.ts that preserves the stack that we're not implementing since it found a local function with the same name.

The original reason for automatically clearing stack traces is that it felt safer than assuming any DIY-SSR setups would always be stripping them on the server like Remix does so it prevented any accidental leakage of SSR stack traces. Including the in dev-only mode felt like something advanced users could achieve via manual hydration.

I think for now we can just de-dup them and maybe add a param to preserve the stack trace that we send from the Remix usage in RouterProvider and continue stripping in the RR case (parseHydrationData).

I am also pretty sure the Remix usage can go away in v7 with single fetch but would need to look a bit deeper into that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pairing with @pcattori, we've opted to go with this to keep things moving:

error.stack = process.env.NODE_ENV === "development" ? val.stack : "";

Deduping feels like a larger task given the current structure so I've left that for now.

@markdalgleish markdalgleish merged commit 421cf56 into v7 Apr 8, 2024
3 of 5 checks passed
@markdalgleish markdalgleish deleted the markdalgleish/fix-error-sanitization branch April 8, 2024 22:21
brophdawg11 added a commit that referenced this pull request Apr 10, 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.

2 participants