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

Remix redirect headers are broken (x-remix-redirect) #2269

Closed
4 tasks done
sebastien-comeau opened this issue Sep 9, 2024 · 11 comments · Fixed by #2284
Closed
4 tasks done

Remix redirect headers are broken (x-remix-redirect) #2269

sebastien-comeau opened this issue Sep 9, 2024 · 11 comments · Fixed by #2284
Assignees
Labels
bug Something isn't working needs:triage Issues that have not been investigated yet. scope:node Related to MSW running in Node

Comments

@sebastien-comeau
Copy link

sebastien-comeau commented Sep 9, 2024

Prerequisites

Environment check

  • I'm using the latest msw version
  • I'm using Node.js version 18 or higher

Node.js version

v20.17.0

Reproduction repository

https://github.com/sebastien-comeau/remix-with-msw

Reproduction steps

Start the application in development mode using npm run dev. Then, click the Go to movies button, which triggers a Remix action and returns a redirect response to /movies. This issue doesn't occur with MSW v2.4.3, so we suspect it was introduced with the @mswjs/interceptors update in #2268.

Current behavior

When redirecting to /movies from the server action, the browser URL is incorrectly set to /movies,%20/movies. The Remix redirect response header shows x-remix-redirect: /movies, /movies.

Expected behavior

The browser URL should correctly display /movies when redirecting from the server action. The Remix redirect response header should be x-remix-redirect: /movies.

@sebastien-comeau sebastien-comeau added bug Something isn't working needs:triage Issues that have not been investigated yet. scope:node Related to MSW running in Node labels Sep 9, 2024
@BraedenKilburn
Copy link

BraedenKilburn commented Sep 9, 2024

I found an issue where my response resolver would access the request body as seen in the docs here:

http.post('/post/:postId', async ({ request, params, cookies }) => {
  const { postId } = params
  const requestBody = await request.json()
})

What I had to do was add clone the request object like so:

http.post('/post/:postId', async ({ request, params, cookies }) => {
  const { postId } = params
  const requestBody = await request.clone().json()
})

This resolved my error. I found this from this issues thread here. I don't know if this will fix your problem but I wanted to share my findings on how to fix an error I had after updating to 2.4.4. An issue with this is that I lose the typings on my request body after I clone the request.

@kettanaito
Copy link
Member

kettanaito commented Sep 9, 2024

Thanks for reporting this.

I believe it's a bug on our end. We need to copy the request's body safely when redirecting with body. This is a task for interceptors.

Edit: This was not related to what I thought it was at all. See the root cause in mswjs/interceptors#631.

@stevensacks
Copy link

stevensacks commented Sep 11, 2024

Can confirm this is happening to me, as well.

export const action: ActionFunction = async ({request}) => {
  const formData = await request.formData();
  await updateThing(formData, request); // fetch to endpoint mocked by MSW
  return redirect('/things', {status: 303});
};

Should redirect from /things/1 to /things, but instead redirects to /things,%20/things

@sndrem
Copy link

sndrem commented Sep 13, 2024

I have the same behavior in my app, as described in the previous comments.

@kettanaito kettanaito changed the title v2.4.4 breaks Remix redirect header Remix redirect headers are broken (x-remix-redirect) Sep 15, 2024
@kettanaito
Copy link
Member

This is also likely caused by mswjs/interceptors#631. Based on the reported broken Remix redirect header x-remix-redirect: /movies, /movies, it looks like the next location is being appended to the header value, instead of replacing it.

@kettanaito
Copy link
Member

Released: v2.4.7 🎉

This has been released in v2.4.7!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

@iamtomcat
Copy link

@kettanaito I'm still seeing the same behavior with the headers being swallowed by the interceptors somewhere.

@kettanaito
Copy link
Member

@iamtomcat, please open a new issue and submit a reproduction repo. This bug has been fixed.

@stevensacks
Copy link

stevensacks commented Sep 17, 2024

@kettanaito Unfortunately, it's not completely fixed. I upgraded to 2.4.8 and some of my endpoints are working (simple CRUD), but my login API call doesn't even make it to the MSW handler, and immediately throws the following error from the interceptors.

TypeError: Cannot set property headers of #<_Request> which has only a getter
      at Object.construct (file:///Users/stevensacks/Development/gaia/framework/remix/node_modules/@mswjs/interceptors/lib/node/chunk-RWGRRMVU.mjs:217:27)
      at Authenticator.authenticate (/Users/stevensacks/Development/gaia/framework/remix/node_modules/remix-auth/build/authenticator.js:68:41)

Downgrading back to 2.4.2 resolves the issue.

@kettanaito
Copy link
Member

The originally reported bug is fixed, there's an automated test to prove that. If you are experiencing a related issue, I understand how it may seem the same on the surface.

Please see this: #2269 (comment)

@stevensacks
Copy link

@kettanaito Ok I've done that. I didn't know how to report the bug exactly, so I just named it after the error message.

#2290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs:triage Issues that have not been investigated yet. scope:node Related to MSW running in Node
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants