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

Remove instanceof Response checks in favor of isResponse #4782

Merged
merged 3 commits into from
Dec 6, 2022

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Dec 6, 2022

Fixes #4741 by using isResponse instead of instanceof Response

Note: integration tests will fail until we get @remix-run/server-runtime onto the latest @remix-run/router

@changeset-bot
Copy link

changeset-bot bot commented Dec 6, 2022

🦋 Changeset detected

Latest commit: f2ac118

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

@brophdawg11 brophdawg11 linked an issue Dec 6, 2022 that may be closed by this pull request
@@ -103,5 +124,12 @@ test.describe("loader in an app", () => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/redirect");
expect(await app.getHtml()).toMatch(HOME_PAGE_TEXT);
expect(await app.getHtml()).toMatch(REDIRECT_TARGET_TEXT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strengthened the existing redirect tests a bit to ensure it renders the right content at the redirected location.

`,
"app/routes/fetch.jsx": js`
export function loader({ request }) {
return fetch(new URL(request.url).origin + '/fetch-target');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the case we're fixing here - returning a raw fetch Response wasn't matching our instanceof Response check

@brophdawg11 brophdawg11 merged commit c72d4ee into release-hotfix Dec 6, 2022
@brophdawg11 brophdawg11 deleted the brophdawg11/remove-instanceof-response branch December 6, 2022 17:05
kentcdodds pushed a commit that referenced this pull request Dec 15, 2022
* Remove instanceof Response checks in favor of isResponse

* Add changeset

* Bump to @remix-run/router 1.0.5-pre.1
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.

Error: Expected a Response to be returned from queryRoute
2 participants