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

Add support for X-Remix-Reload-Document header #6842

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

robbtraister
Copy link
Contributor

This PR is paired with remix-run/react-router#10705 to add support for passing along the X-Remix-Reload-Document header.

Open Development discussion

@changeset-bot
Copy link

changeset-bot bot commented Jul 14, 2023

⚠️ No Changeset found

Latest commit: 19e9291

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

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

@robbtraister robbtraister marked this pull request as ready for review July 14, 2023 20:22
@zackerydev
Copy link

Any idea when this is going to land? Need any help getting this in

@gerbyzation
Copy link

@zackeryg see remix-run/react-router#10705 (comment)

@brophdawg11
Copy link
Contributor

Thanks @robbtraister! We updated to an experimental react router version in #7040 so this is good to merge to dev and ship in Remix v2. I added an integration test as well so I'll get this merge once CI passes.

Comment on lines 258 to +264
if (revalidate) {
headers["X-Remix-Revalidate"] = revalidate;
}
let reloadDocument = response.headers.get("X-Remix-Reload-Document");
if (reloadDocument) {
headers["X-Remix-Reload-Document"] = reloadDocument;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brophdawg11 do you think it's worth refactoring this into an array + for-loop (or .forEach or .reduce)?

something like:

let HEADERS_TO_PRESERVE = ["X-Remix-Revalidate", "X-Remix-Reload-Document"];

...

  for (let headerName of HEADERS_TO_PRESERVE) {
    let headerValue = response.headers.get(headerName);
    if (headerValue) {
      headers[headerName] = headerValue;
    }
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

nah not yet for the 2nd header - something something kent c dodds avoid hasty abstractions :)

@brophdawg11 brophdawg11 merged commit 50cdfec into remix-run:dev Aug 22, 2023
9 checks passed
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-49e8da1-20230823 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!

@robbtraister robbtraister deleted the feature/redirect-with-reload branch August 23, 2023 16:55
@github-actions
Copy link
Contributor

🤖 Hello there,

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

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.

None yet

5 participants