Handle case when session storage is blocked #10848
Conversation
🦋 Changeset detectedLatest commit: b80680a The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
|
Hi @david-bezero, Welcome, and thank you for contributing to React Router! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
packages/react-router-dom/index.tsx
Outdated
| } catch (error) { | ||
| console.warn("Failed to record scroll position in session storage. Scroll restoration will not work.", error); | ||
| } |
There was a problem hiding this comment.
We have a built-in warning method we can use here:
| } catch (error) { | |
| console.warn("Failed to record scroll position in session storage. Scroll restoration will not work.", error); | |
| } | |
| } catch (error) { | |
| warning( | |
| false, | |
| "Failed to save scroll positions in sessionStorage, <ScrollRestoration /> will not work properly." | |
| ); | |
| console.error(error); | |
| } |
There was a problem hiding this comment.
why call console.error? I guess it's to make sure the cause is displayed somewhere, but since we can recover from the situation I don't think it should be reported as an error - could it remain console.warn?
There was a problem hiding this comment.
Yeah the idea there was to show the actual underlying error, but we could probably inline error.message into the warning as well if we wanted to keep just one console entry and avoid a console.error.
There was a problem hiding this comment.
Added it using message (${error}). format, following an existing convention from safelyDecodeURI. This also avoids any issues if the thrown entity is not an object for whatever reason.
packages/react-router-dom/index.tsx
Outdated
| storageKey || SCROLL_RESTORATION_STORAGE_KEY, | ||
| JSON.stringify(savedScrollPositions) | ||
| ); | ||
| window.history.scrollRestoration = "auto"; |
There was a problem hiding this comment.
We should probably keep this outside the try so it gets properly reset even if the sessonStorage set fails
|
I also just noticed this is pointed to |
david-bezero
force-pushed
the
handle-session-storage-blocked
branch
from
a7fba64
to
80c2f60
Compare
|
updated the logging, rebased, repointed, and added a changeset |
|
Thanks for the PR! |
|
🤖 Hello there, We just published version Thanks! |
|
🤖 Hello there, We just published version Thanks! |
As discussed in #10842
This writes a descriptive warning to the console if
sessionStorageis unavailable for any reason, but allows the scroll saving/resetting to continue working as much as possible (until the user leaves the page we still have the scroll positions in a global variable)It also retro-fits a "happy-path" test for
ScrollRestorationto confirm the scroll position is correctly reset and no warnings are printed.Interestingly, writing the test turned up a behaviour quirk where the default
getKeyimplementation fails to save the scroll position for the first page viewed (since itslocation.keyisdefaulton the first visit). I haven't attempted to fix this; I'm not even sure if it's a real issue outside the test environment.