Omit search part of path when detecting if the route changed after downloading asset bundles #6707

n8agrin · 2023-06-27T17:23:52Z

Resolves #6703

Previous implementation used the pathname and search parts of a url to determine if route changed, which causes some conflict when CDNs / cache proxies are placed between the Remix app and the client. This change allows remix to reload the page if only the path changes. There may be some issues with how loaders are handled when configured behind a CDN, since Remix expects to trigger loaders when the search part of a url changes, but the total impact here is dependent on how a developer is configuring the network topology on top of a Remix server.

Closes: #6703

Docs
Tests

Testing Strategy:

…wnloading asset bundles Resolves remix-run#6703 Previous implementation used the pathname and search parts of a url to determine if route changed, which causes some conflict when CDNs / cache proxies are placed between the Remix app and the client. This change allows remix to reload the page if only the path changes. There may be some issues with how loaders are handled when configured behind a CDN, since Remix expects to trigger loaders when the search part of a url changes, but the total impact here is dependent on how a developer is configuring the network topology on top of a Remix server.

changeset-bot · 2023-06-27T17:23:55Z

🦋 Changeset detected

Latest commit: 9c6cf2f

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

This PR includes changesets to release 18 packages

Name	Type
@remix-run/react	Patch
@remix-run/server-runtime	Patch
@remix-run/testing	Patch
@remix-run/cloudflare	Patch
@remix-run/deno	Patch
@remix-run/dev	Patch
@remix-run/node	Patch
@remix-run/cloudflare-pages	Patch
@remix-run/cloudflare-workers	Patch
create-remix	Patch
@remix-run/architect	Patch
@remix-run/express	Patch
@remix-run/netlify	Patch
@remix-run/serve	Patch
@remix-run/vercel	Patch
remix	Patch
@remix-run/css-bundle	Patch
@remix-run/eslint-config	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

n8agrin · 2023-06-27T17:28:03Z

@yarbsemaj @brophdawg11 Please see the note in the description. I am concerned about the impact this might have on loaders which expect specific search params to function correctly. I'm also not 100% sure I understand your use case @yarbsemaj.

Please confirm the issue is something like:

user visits /foo?a=yes
cdn caches /foo?a=yes
user visits /foo
cdn loads cached assets including the serialized /foo?a=yes string
remix redirects to /foo
💥 the cycle repeats itself

I suspect the better solution here might be to change your cdn cache policy to ignore search params in the url.

BillBrower-Shopify · 2023-06-28T14:42:50Z

This ends up causing an infinite reload cycle for urls without query params when I test it as a patch on top of Remix 1.18.0 in our project.

Edit: The infinite reload was actually caused by the patch being correctly applied to @remix-run/server-runtime but not @remix-run/react. After correcting the @remix-run/react patch this works as expected and there is no infinite reload issue.

yarbsemaj · 2023-06-28T14:58:55Z

@n8agrin Unfortunately this is not an option in our setup, and i imagine many other as well

n8agrin · 2023-06-28T15:15:57Z

@yarbsemaj ok. I’m not a fan of this approach. Closing. @brophdawg11 is the expert here as far as I can tell. I’ll wait for @brophdawg11 to weigh in

brophdawg11 · 2023-06-29T14:43:31Z

Unfortunately this is not an option in our setup

@yarbsemaj This is a bit ambiguous to me. Is the pathname-only comparion implemented in this PR not an option for your setup?

Or were you referring to the solution suggested by @n8agrin above:

I suspect the better solution here might be to change your cdn cache policy to ignore search params in the url.

From the description of your issue in #6703 it sounds like the root cause of your issue is the inclusion of search params in the hydration check, and that only comparing pathnames would suffice?

brophdawg11 · 2023-06-29T16:41:38Z

I'm going to re-open this since I do believe this is the right solution to avoid reloads when CDN's ignore certain search parameters. I confirmed with @BillBrower-Shopify that this fix resolves the issue they are seeing which is due to the CDN only respecting certain search params. While this is sort of a non-standard HTTP caching approach, I believe it's probably used pretty frequently.

Since the underlying issue here is a mismatch in matched routes on the server and the client, and route matching only cares about the pathname, I believe this is how the original implementation should have been anyway.

n8agrin · 2023-06-29T17:21:41Z

Thanks for weighing in @brophdawg11

brophdawg11 · 2023-06-29T18:57:41Z

packages/remix-dev/__tests__/cli-test.ts

@@ -230,7 +230,7 @@ describe("remix CLI", () => {
      await interactWithShell(proc, [
        { question: /Where.*create.*app/i, type: [projectDir, ENTER] },
        { question: /What type of app/i, answer: /basics/i },
-        { question: /Where.*deploy/i, answer: /express/i },
+        { question: /Where.*deploy/i, answer: /remix/i },


Ignore this unit test fix - this was fixed in dev and cherry-picked into main and release-next. This branch has the main SHA since I merged main in - but this code is already in release-next so it's not actually changing anything.

brophdawg11 · 2023-06-29T19:18:27Z

packages/remix-server-runtime/server.ts

@@ -335,7 +335,7 @@ async function handleDocumentRequestRR(
      ...entryContext,
      staticHandlerContext: context,
      serverHandoffString: createServerHandoffString({
-        url: context.location.pathname + context.location.search,
+        url: context.location.pathname,


Keeping the url name in this handoff string since it's technically exposed to users via EntryContext.serverHandoffString and window.__remixContext so just in case anyone is accessing it.

jacob-ebey · 2023-06-29T19:09:21Z

packages/remix-server-runtime/server.ts

@@ -335,7 +335,7 @@ async function handleDocumentRequestRR(
      ...entryContext,
      staticHandlerContext: context,
      serverHandoffString: createServerHandoffString({
-        url: context.location.pathname + context.location.search,
+        pathname: context.location.pathname,


Do we consider this handoff format public API? If so can we not re-name the property here and re-purpose the "url" name?

Updated in 9c6cf2f

github-actions · 2023-06-29T21:12:31Z

🤖 Hello there,

We just published version 1.18.1-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!

github-actions · 2023-06-30T07:05:56Z

🤖 Hello there,

We just published version v0.0.0-nightly-0657c16-20230630 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!

yarbsemaj · 2023-06-30T08:49:05Z

@brophdawg11

Unfortunately this is not an option in our setup

@yarbsemaj This is a bit ambiguous to me. Is the pathname-only comparion implemented in this PR not an option for your setup?

Sorry for the confusion, i was referring to this comment about updating our CDN Policy, this is due to us needing access to a full range of query parameters on certan, arbitrary routes that can not easily be modeled with cache behaviours.

I suspect the better solution here might be to change your cdn cache policy to ignore search params in the url.

brophdawg11 · 2023-06-30T14:26:16Z

@yarbsemaj No worries! Can you confirm if 1.18.1-pre.0 fixes your issue?

github-actions · 2023-06-30T18:40:19Z

🤖 Hello there,

We just published version 1.18.1-pre.2 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 · 2023-06-30T21:13:19Z

🤖 Hello there,

We just published version 1.18.1 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!

remix-cla-bot bot added the CLA Signed label Jun 27, 2023

n8agrin mentioned this pull request Jun 27, 2023

CDN Causing infinite reload #6703

Closed

1 task

n8agrin closed this Jun 28, 2023

brophdawg11 reopened this Jun 29, 2023

markdalgleish and others added 2 commits June 29, 2023 14:52

test(dev): fix CLI test (remix-run#6698)

d08708f

Merge branch 'main' into nate/dev-fix-6703

230c727

brophdawg11 changed the base branch from main to release-next June 29, 2023 18:55

brophdawg11 reviewed Jun 29, 2023

View reviewed changes

Add changeset

c737392

brophdawg11 added package:server-runtime renderer:react labels Jun 29, 2023

Keep handoff url property name

9c6cf2f

brophdawg11 reviewed Jun 29, 2023

View reviewed changes

jacob-ebey approved these changes Jun 29, 2023

View reviewed changes

brophdawg11 merged commit c8db245 into remix-run:release-next Jun 29, 2023
9 checks passed

github-actions bot mentioned this pull request Jun 29, 2023

chore: Update version for release (pre) #6726

Merged

github-actions bot mentioned this pull request Jun 30, 2023

chore: Update version for release #6742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omit search part of path when detecting if the route changed after downloading asset bundles #6707

Omit search part of path when detecting if the route changed after downloading asset bundles #6707

n8agrin commented Jun 27, 2023

changeset-bot bot commented Jun 27, 2023 •

edited

Loading

n8agrin commented Jun 27, 2023

BillBrower-Shopify commented Jun 28, 2023 •

edited

Loading

yarbsemaj commented Jun 28, 2023

n8agrin commented Jun 28, 2023

brophdawg11 commented Jun 29, 2023

brophdawg11 commented Jun 29, 2023

n8agrin commented Jun 29, 2023

brophdawg11 Jun 29, 2023

brophdawg11 Jun 29, 2023

jacob-ebey Jun 29, 2023

brophdawg11 Jun 29, 2023

github-actions bot commented Jun 29, 2023

github-actions bot commented Jun 30, 2023

yarbsemaj commented Jun 30, 2023 •

edited

Loading

brophdawg11 commented Jun 30, 2023

github-actions bot commented Jun 30, 2023

github-actions bot commented Jun 30, 2023

Omit search part of path when detecting if the route changed after downloading asset bundles #6707

Omit search part of path when detecting if the route changed after downloading asset bundles #6707

Conversation

n8agrin commented Jun 27, 2023

changeset-bot bot commented Jun 27, 2023 • edited Loading

🦋 Changeset detected

n8agrin commented Jun 27, 2023

BillBrower-Shopify commented Jun 28, 2023 • edited Loading

yarbsemaj commented Jun 28, 2023

n8agrin commented Jun 28, 2023

brophdawg11 commented Jun 29, 2023

brophdawg11 commented Jun 29, 2023

n8agrin commented Jun 29, 2023

brophdawg11 Jun 29, 2023

Choose a reason for hiding this comment

brophdawg11 Jun 29, 2023

Choose a reason for hiding this comment

jacob-ebey Jun 29, 2023

Choose a reason for hiding this comment

brophdawg11 Jun 29, 2023

Choose a reason for hiding this comment

github-actions bot commented Jun 29, 2023

github-actions bot commented Jun 30, 2023

yarbsemaj commented Jun 30, 2023 • edited Loading

brophdawg11 commented Jun 30, 2023

github-actions bot commented Jun 30, 2023

github-actions bot commented Jun 30, 2023

changeset-bot bot commented Jun 27, 2023 •

edited

Loading

BillBrower-Shopify commented Jun 28, 2023 •

edited

Loading

yarbsemaj commented Jun 30, 2023 •

edited

Loading