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

[Bug?]: 404 on paths containing periods #9969

Closed
1 task
pvenable opened this issue Feb 6, 2024 · 17 comments · Fixed by #10114
Closed
1 task

[Bug?]: 404 on paths containing periods #9969

pvenable opened this issue Feb 6, 2024 · 17 comments · Fixed by #10114
Labels
bug/confirmed We have confirmed this is a bug topic/fastify Fastify

Comments

@pvenable
Copy link
Contributor

pvenable commented Feb 6, 2024

What's not working?

We're seeing 404s instead of index.html for paths containing periods, e.g. https://example.com/foo/bar.baz. Our app has profile handles that can include periods and that are used directly in urls like this.

I'm investigating whether we have any way to override this, so if there's a known workaround, that would be great to hear. This is a live production issue for us right now.

We're also seeing evidence that reverting to Redwood 6.5.1 or so might work as a temporary solution.

How do we reproduce the bug?

Create a new redwood app and generate a page:

yarn create redwood-app periods-break-urls --typescript
cd periods-break-urls
yarn rw g page Test

Update web/src/Routes.tsx so the path contains a period:

-      <Route path="/test" page={TestPage} name="test" />
+      <Route path="/test.profile" page={TestPage} name="test" />

Run the app:

yarn rw dev

or if you prefer

yarn rw build && yarn rw serve

Visit http://localhost:8910/test.profile

Expected result: Test page renders
Actual result: 404 Not Found

What's your environment? (If it applies)

Redwood 6.6.4

Are you interested in working on this?

  • I'm interested in working on this
@pvenable pvenable added the bug/needs-info More information is needed for reproduction label Feb 6, 2024
@dthyresson
Copy link
Contributor

Hi @pvenable and thanks for bringing this to our attention. Might this be similar behavior as #9663 ?

@pvenable
Copy link
Contributor Author

pvenable commented Feb 7, 2024

Might this be similar behavior as #9663 ?

I think it's similar, yes, but I raised a separate issue since that one is specifically discussing query params, where we're seeing the problem directly in the path. I also wasn't sure whether that issue had been addressed, since it's still open but I also see #9714.

@pvenable
Copy link
Contributor Author

pvenable commented Feb 7, 2024

I originally thought #9272 caused this, but I realize now that that was released in 6.3.3, and this issue is not present yet even on 6.5.1 (which we reverted to as a workaround). So now I see that this problem was introduced in 6.6.0 -- perhaps indeed by #9714, which was released then with intent to fix #9663.

@dthyresson dthyresson added bug/confirmed We have confirmed this is a bug topic/fastify Fastify and removed bug/needs-info More information is needed for reproduction labels Feb 13, 2024
@pvenable
Copy link
Contributor Author

I see this has been labeled a confirmed bug. Is there a way to override this behavior in the meantime, or will we have to stick to 6.5.1 until this can be resolved directly?

I don't have sufficient context on the changes that caused this issue, so at the moment I'm not sure I could suggest a fix other than to revert the changes entirely, which would presumably break (new?) intended behavior.

@dthyresson
Copy link
Contributor

Could you use some other delimiter than a period?

@dthyresson
Copy link
Contributor

For context, there was a change in some extension detection that makes periods act like an extension delimiter:

const requestedExtension = path.extname(req.url)

Thus the route could be not found when parsed.

@pvenable
Copy link
Contributor Author

Could you use some other delimiter than a period?

Unfortunately we have a live production application with existing usernames and profile pages I don't think we're planning to change, so for now we'll probably have to just stick to 6.5.1 until this is resolved.

there was a change in some extension detection that makes periods act like an extension delimiter

This seems like a rather significant breaking change for a minor. I'm not sure I understand why this was necessary to do at such a global scope rather than at e.g. a particular (configurable?) base path, but in any case, we don't want this behavior (at least not globally), so hopefully there will be a way to opt in or out at some point.

@pvenable
Copy link
Contributor Author

pvenable commented Mar 1, 2024

We're pretty motivated to stay current on Redwood and we're eager to take advantage of RSC/SSR when the time comes, but we can't upgrade past 6.5.1 while this bug remains.

What can I do to help get this resolved? Can the change that introduced this be reverted, or is there a clear direction for how to fix this while retaining whatever functionality drove that change?

@Tobbe
Copy link
Member

Tobbe commented Mar 2, 2024

I'll take a look at this tomorrow. I haven't been following along, so first I will have to get up to speed on things. Then I'll let you know how we'll handle this

@Tobbe
Copy link
Member

Tobbe commented Mar 3, 2024

This seems like a rather significant breaking change for a minor.

Yeah, this was/is not how we'd label this had we realized this would be the effect. Redwood definitely needs to support dots in the url path, just like you have it. I'll have to do a deeper dive to figure out exactly how to fix this, but it should definitely be fixed somehow.

So next step here is for me to get a reproduction up locally (thanks for providing the steps on how to do so!) and then start looking into a solution. I'll try to get to it tomorrow (Monday) or early Tuesday. After that I'll be traveling.

@Tobbe
Copy link
Member

Tobbe commented Mar 4, 2024

Sorry, bad news. This is a bug in vite 🙁

vitejs/vite#2415 (comment)

It's fixed in Vite 5, but we can't upgrade because we don't fully support ESM yet, and Vite 5 dropped CJS support.

However that only explains it for yarn rw dev. Tomorrow I'll look into yarn rw serve

@Tobbe
Copy link
Member

Tobbe commented Mar 5, 2024

For the yarn rw serve part of the equation, I also landed on #9272 as being the culprit. The code has moved to a new file now, but still references that same PR

const requestedExtension = path.extname(urlData.path ?? '')
// Paths with no extension (`/about`) or an .html extension (`/about.html`)
// should be handled by the client side router.
// See the discussion in https://github.com/redwoodjs/redwood/pull/9272.
if (requestedExtension === '' || requestedExtension === '.html') {
reply.header('Content-Type', 'text/html; charset=UTF-8')
return reply.sendFile(fallbackIndexPath)
}

Worth noting also that this is only an issue if the part of the url that can have a period is last.

So, from the repo example in the issue description:
This will fail

<Route path="/test.profile" page={TestPage} name="test" />

This will work

<Route path="/test.profile/dashboard" page={TestPage} name="test" />

@Tobbe
Copy link
Member

Tobbe commented Mar 6, 2024

@pvenable A potential fix for this was released in v7.0.7. Could you please try that version and report back if it works for you?

@pvenable
Copy link
Contributor Author

pvenable commented Mar 6, 2024

@Tobbe Thanks for the fix! I'll report results as soon as I can as I'm currently working through other issues upgrading to v7, some of which don't seem to be referenced in the upgrade guide.


Completely off topic for this issue but I'm getting errors with graphql fragments while attempting to upgrade, so first I need to figure out if it's a Redwood issue or a conflict specific to our codebase. I can open a new issue if it's the former, but just for context I'm getting errors wherever we're including fragments into other queries/fragments roughly like:

const fragment = gql`
  fragment ExampleFragment on Whatever {
    id
    ...SomeOtherComponentFragment
  }

${SomeOtherComponent.fragments.foo}
`

with error messages like Expected 1 arguments, but got 2 on the line ${SomeOtherComponent.fragments.foo}. With multiple fragments the number of arguments received increases accordingly, so I think it's related to the gql tag calls.

I also see this error in my editor when navigating to the gql tag in node_modules/@redwoodjs/web/dist/global.web-auto-imports.d.ts, so I'm a bit suspicious it's framework-related, but I haven't ruled out a project-specific issue yet:

image

(It looks like the gql tag resolved to node_modules/graphql-tag/src/index.ts on Redwood v6.5.1.)

Anyway, apologies for going off-topic here. I'll probably open an issue for this if I don't get it resolved soon.

@pvenable
Copy link
Contributor Author

pvenable commented Mar 7, 2024

I've opened an issue (with repro) for the graphql fragment error above in #10120, but I look forward to testing the path fix once I can upgrade our app. Thanks again!

@dthyresson
Copy link
Contributor

@pvenable FYI I replied to issue #10120 regarding use of fragments with v7 and the new documentation for setup: yarn rw setup graphql fragments

@pvenable
Copy link
Contributor Author

pvenable commented Mar 8, 2024

@Tobbe I'm happy to report that the v7.0.7 release appears to resolve the paths-with-periods issue (outside of the dev server). 🎉

I understand that the issue persists with yarn rw dev because of a vite bug, and we can live with that.

Thanks for the resolution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug topic/fastify Fastify
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants