Add callsite revalidation optout

[dadamssg]

This adds callsite revalidation optout as brought up here: #10006

<Form method='post' defaultShouldRevalidate={false}>

fetcher.submit(target, {
  method: 'post',
  defaultShouldRevalidate: false
}

If this new value evaluates to false, it will set that as the defaultShouldRevalidate value that's sent to the shouldRevalidate function.

[changeset-bot]

🦋 Changeset detected

Latest commit: dae1b3d

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

This PR includes changesets to release 11 packages

Name	Type
react-router	Patch
@react-router/architect	Patch
@react-router/cloudflare	Patch
@react-router/dev	Patch
react-router-dom	Patch
@react-router/express	Patch
@react-router/node	Patch
@react-router/serve	Patch
@react-router/fs-routes	Patch
@react-router/remix-routes-option-adapter	Patch
create-react-router	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

[brophdawg11]

General approach is looking good on a quick pass! A few minor comments - I'll do some deeper testing once those are updated

[dadamssg]

Updated per your comments. good calls 👍

[brophdawg11]

This is looking awesome! I made some very minor updates inside the router, and instead of testing locally with an app I just wrote a bunch of tests and they all passed so I think we're good on that front. I prefixed it with unstable_ for initial release. I'll get an experimental cut or some early alpha testing in a bit here.

[brophdawg11]

This is the only functional change - we also need to apply this to any fetcher.load revalidation checks

[brophdawg11]

I just kicked off an experimental release from this branch for some alpha testing: 0.0.0-experimental-2d8f5c807

[dadamssg]

woohoo! Thanks for carrying it to the finish line @brophdawg11!

[brookslybrand]

Am I missing something or holding it wrong?

stackblitz

wat.yafw.balanced.mp4

[dadamssg]

ooh good catch @brookslybrand. It's because you're actually exporting a shouldRevalidate in the route. We I didn't check for that 🙃 It looks like Matt did add a test explicitly for that. I'll have to dig into why the test behaves differently.

Problem is here @brophdawg11.

react-router/packages/react-router/lib/dom/ssr/routes.tsx

Lines 607 to 613 in a947b03

	// Single fetch revalidates by default, so override the RR default value which
	// matches the multi-fetch behavior with `true`
	if (ssr && route.shouldRevalidate) {
	let fn = route.shouldRevalidate;
	return (opts: ShouldRevalidateFunctionArgs) =>
	fn({ ...opts, defaultShouldRevalidate: true });
	}

The existence of this code and the comment makes me believe it might not be as simple as just deleting though. 🤔

[brophdawg11]

ah, yeah good catch - this is a bug just in the framework mode/single fetch layer, which is why it's not showing up in our data mode tests. We'll need to get e2e tests in as well that can run through this single fetch logic. I've never liked this particular line of code (yes I wrote it 🙃 ) and the issue boils down to the ability to supply a custom dataStrategy but not a custom defaultShouldRevalidate (at the moment).

Let me poke and see if we can do this properly in dataStrategy via shouldCallHandler, and if not we may need to investigate a new defaultShouldRevalidate() createRouter option alongside dataStrategy.

[brophdawg11]

lol so it looks like we actually already did this correctly when we implemented unstable_shouldCallHandler (a more powerful replacement for dataStrategy's shouldLoad):

react-router/packages/react-router/lib/dom/ssr/single-fetch.tsx

Lines 387 to 391 in a947b03

	let defaultShouldRevalidate =
	!m.unstable_shouldRevalidateArgs ||
	m.unstable_shouldRevalidateArgs.actionStatus == null ||
	m.unstable_shouldRevalidateArgs.actionStatus < 400;
	let shouldCall = m.unstable_shouldCallHandler(defaultShouldRevalidate);

I think we just forgot to remove the old way of doing it (the above snippet causing the issue here). On a quick look deleting those lines that hardcode it to true doesn't break any single fetch E2E tests and fixes the issue at hand here. I'll dig in a bit more and if it looks right I'll push the change up here.

[brophdawg11]

#14592 removes the above snippet causing the issue and stabilizes the lower level unstable_shouldCallHandler APIs that allow this call-site override to work with custom data strategies 👍

[brophdawg11]

I repointed this to dev and merged latest dev in with the fix for the issue @brookslybrand called out above and released a new experimental in 0.0.0-experimental-112589a91. The linked stackblitz looks good on that version 👍

[brophdawg11]

I think this is good to merge, but I just want to give 7.10.0 a few days in the wild to make sure no one reports any issues from the upstream data strategy changes in #14592. I'll plan to get this merged later this week or early next week for the next release.