redwood icon indicating copy to clipboard operation
redwood copied to clipboard

[Bug?]: 404 on paths containing periods

Open pvenable opened this issue 1 year ago • 7 comments

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 avatar Feb 06 '24 23:02 pvenable

Hi @pvenable and thanks for bringing this to our attention. Might this be similar behavior as https://github.com/redwoodjs/redwood/issues/9663 ?

dthyresson avatar Feb 07 '24 04:02 dthyresson

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 avatar Feb 07 '24 16:02 pvenable

I originally thought https://github.com/redwoodjs/redwood/pull/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.

pvenable avatar Feb 07 '24 22:02 pvenable

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.

pvenable avatar Feb 15 '24 16:02 pvenable

Could you use some other delimiter than a period?

dthyresson avatar Feb 15 '24 17:02 dthyresson

For context, there was a change in some extension detection that makes periods act like an extension delimiter: https://github.com/redwoodjs/redwood/blob/a231840655906f57996bcf33ac0fb6e76657d7bc/packages/api-server/src/plugins/withWebServer.ts#L60

Thus the route could be not found when parsed.

dthyresson avatar Feb 15 '24 17:02 dthyresson

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 avatar Feb 15 '24 18:02 pvenable

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?

pvenable avatar Mar 01 '24 23:03 pvenable

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 avatar Mar 02 '24 00:03 Tobbe

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 avatar Mar 03 '24 21:03 Tobbe

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

https://github.com/vitejs/vite/issues/2415#issuecomment-1720814355

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 avatar Mar 04 '24 22:03 Tobbe

For the yarn rw serve part of the equation, I also landed on https://github.com/redwoodjs/redwood/pull/9272 as being the culprit. The code has moved to a new file now, but still references that same PR

https://github.com/redwoodjs/redwood/blob/0b1b4c6218c4d391b9cdd2288bbb058ca9d836ae/packages/adapters/fastify/web/src/web.ts#L87-L95

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 avatar Mar 05 '24 08:03 Tobbe

@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?

Tobbe avatar Mar 06 '24 21:03 Tobbe

@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 avatar Mar 06 '24 23:03 pvenable

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!

pvenable avatar Mar 07 '24 01:03 pvenable

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

dthyresson avatar Mar 07 '24 07:03 dthyresson

@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!

pvenable avatar Mar 08 '24 16:03 pvenable