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

feat(remix-server-runtime): RRR 1.3 / 1.4 - handleDocumentRequest #4385

Merged
merged 32 commits into from
Nov 15, 2022

Conversation

jacob-ebey
Copy link
Member

@jacob-ebey jacob-ebey commented Oct 18, 2022

decision doc

Part 3 and 4 of step 1 in layering Remix back on top of React Router now that we have released @remix-run/router.

Changes:

  • Implement handleDocumentRequest on top of RR alongside existing implementation
  • Disambiguate existing internal errors surfacing to CatchBoundary

Closes: #

  • Docs
  • Tests

Testing Strategy:

Assert that existing tests pass and manually check new code is not in non-experimental production builds.

@changeset-bot
Copy link

changeset-bot bot commented Oct 18, 2022

🦋 Changeset detected

Latest commit: ccae87f

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

This PR includes changesets to release 16 packages
Name Type
@remix-run/server-runtime Patch
@remix-run/cloudflare Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/node Patch
@remix-run/react 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/vercel Patch
@remix-run/serve Patch
remix 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

test: stop compiler test from logging
test: revert mdx test to cover failure case
@sergiodxa
Copy link
Member

Just curious, what's this going to do? Remix already serves document requests for SSR so I suppose is another thing

@brophdawg11 brophdawg11 marked this pull request as ready for review November 1, 2022 18:55
@machour
Copy link
Collaborator

machour commented Nov 1, 2022

@brophdawg11 is there a decision doc for this work? Would help me follow along

@brophdawg11
Copy link
Contributor

@machour Yep! https://github.com/remix-run/remix/blob/main/decisions/0007-remix-on-react-router-6-4-0.md - this PR is steps 1.iii/1.iv under Decision - steps 1.i/1.ii were already merged in #4245 and #4359 (behind the compile-time flag)

@brophdawg11 brophdawg11 mentioned this pull request Nov 14, 2022
5 tasks
@brophdawg11
Copy link
Contributor

Need to add a changeset to describe the changes in error/catch boundary behaviors in Remix. Merging this PR will enable those even without the flag enabled.

@brophdawg11
Copy link
Contributor

I think this is good to go now? Let's touch base with @chaance and make sure that we're good to merge this (since he already cut the 1.7.6 release branch). This code in dev, without the feature flag enabled, would just result in the minor fixes to catch/error boundary handling and could be part of a 1.7.7 if needed. But the plan is to enable the feature flag for a 1.8.0-pre.0 later this week or early next week.

@jacob-ebey jacob-ebey changed the title initial work for document requests feat(remix-server-runtime): RRR 1.3 - handleDocumentRequest Nov 15, 2022
@jacob-ebey jacob-ebey changed the title feat(remix-server-runtime): RRR 1.3 - handleDocumentRequest feat(remix-server-runtime): RRR 1.3 / 1.4 - handleDocumentRequest Nov 15, 2022
@jacob-ebey jacob-ebey merged commit 9f3bfe4 into dev Nov 15, 2022
@jacob-ebey jacob-ebey deleted the rrr-document-requests-1 branch November 15, 2022 23:30
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Nov 15, 2022
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-47b5b54-20221116 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!

kentcdodds pushed a commit that referenced this pull request Dec 15, 2022
)

test: update test to be more reliable
chore: be more protective when accessing route module
test: stop compiler test from logging
feat: disambiguate catch/error boundary responses
fix: ensure route modules are loaded regardless of error state
test: make fetcher tests more reliable
feat: Change missing loader from 405 -> 400 error

Co-authored-by: Matt Brophy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue has been fixed and will be released soon CLA Signed
Projects
No open projects
Status: 🚀 Done
Development

Successfully merging this pull request may close these issues.

4 participants