feat: remove singleFetch flag #11522
Conversation
🦋 Changeset detectedLatest commit: 6c1e2d8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 |
jacob-ebey
force-pushed
the
remove_single_fetch_flag
branch
from
6fc780d
to
0907f61
Compare
integration/resource-routes-test.ts
Outdated
| @@ -208,7 +208,9 @@ test.describe("loader in an app", async () => { | |||
| expect(await res.text()).toEqual("Partial"); | |||
| }); | |||
|
|
|||
| test("should handle objects returned from resource routes", async ({ | |||
| // TODO: Matt and I chatted about this recently, revisit that chat and | |||
There was a problem hiding this comment.
@brophdawg11 I'm disabling this and we can update / re-enable it after your PR goes in.
There was a problem hiding this comment.
yeah this should work after remix-run/remix#9349 is brought over - wanted to do that after this was merged to avoid conflicts
| !(staticHandlerContext instanceof Response), | ||
| "Expected a context" | ||
| ); | ||
| // TODO: figure out why this test exists |
There was a problem hiding this comment.
I'm not sure why these exist but the underlying issue causing them to fail should probably be fixed.
There was a problem hiding this comment.
These tests were from an issue a while back where IIRC the lack of a default export was being converted to export default {} in the compiler and that was causing us to think it had a component export and eventually call createElement({}) - so we strengthened our check for a component to make sure it was an actual react component. I don't specifically recall if they were esbuild only or also a vite thing though.
I got the tests passing and pushed it up
remix-run/remix#7593 (comment)
https://github.com/remix-run/remix/blob/main/packages/remix-react/routes.tsx#L614
remix-run/remix#7745
| return `__remixContext.r(${JSON.stringify(routeId)}, ${JSON.stringify( | ||
| key | ||
| )}, ${escapeHtml(serializedData)})`; | ||
| }; |
| ? new URL( | ||
| reqUrl, | ||
| typeof window === "undefined" | ||
| ? // TODO: Trace usage of singleFetchUrl and make sure we don't use it anywhere | ||
| // this will make it to a network call. | ||
| "server://singlefetch/" | ||
| : window.location.origin | ||
| ) |
There was a problem hiding this comment.
Do you recall what needed this? I removed it and things seem to be passing...
Since it's part of the DOM code it should always generally be running in a jsdom environment I think?
There was a problem hiding this comment.
I removed this in d16ec89 - feel free to revert though if we need to keep it
There was a problem hiding this comment.
found the one integration test that was failing - PrefetchPageLinks calls it during SSR so I added it back with a comment. The other 2 uses of it do result in a network call but both are in the browser so window will be available.
| @@ -1,20 +1,73 @@ | |||
| import type { EntryContext } from "@react-router/node"; | |||
| import { PassThrough } from "node:stream"; | |||
There was a problem hiding this comment.
I think we can just remove this file entirely and the condition that uses it in the remix dev and remix reveal code?
https://github.com/remix-run/react-router/blob/v7/packages/remix-dev/config.ts#L493
https://github.com/remix-run/react-router/blob/v7/packages/remix-dev/cli/commands.ts#L148
There was a problem hiding this comment.
Ah I see what you did - you got rid of the prohibitOutOfOrderStreaming check and just made the spa version always use handleBotRequest. IMO, it would be fine to just delete the SPA version and let npx remix reveal expose the default node one with the prohibitOutOfOrderStreaming check
There was a problem hiding this comment.
Took a stab at that in dd6a86c - feel free to revert if we want to keep it 👍
There was a problem hiding this comment.
hm - so that broke a handful of e2e tests so I reverted for now :)
| } | ||
| } else { | ||
| statusCode = context.statusCode; | ||
| headers = getDocumentHeaders(build, context); |
There was a problem hiding this comment.
I also removed the now unused headers.ts file and the set-cookie-parser dependency in 531b286
chore: add changeset
No description provided.