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

Routing error when navigating to path that includes an encoded forward slash #7962

Closed
1 task
svensigmond opened this issue Aug 4, 2023 · 16 comments
Closed
1 task
Labels
5.0 beta Related to the Astro 5.0 beta - P3: minor bug An edge case that only affects very specific usage (priority) needs triage Issue needs to be triaged

Comments

@svensigmond
Copy link

svensigmond commented Aug 4, 2023

What version of astro are you using?

2.10.1

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

npm

What operating system are you using?

Windows

What browser are you using?

Firefox

Describe the Bug

Given this folder structure:

src
-pages
--currencies
---[id].astro

When navigating to a route with an encoded forward slash, for example: http://foo.com/currencies/eur%2Fusd, the server returns a type error:

Expected "id" to be a string

dist/index.js:237:19


Stack Trace
TypeError: Expected "id" to be a string
    at Object.generate (project_path\node_modules\path-to-regexp\dist\index.js:237:19)
    at stringifyParams (project_path/packages/client/node_modules/astro/dist/core/routing/params.js:23:31)
    at findPathItemByKey (project_path/packages/client/node_modules/astro/dist/core/render/route-cache.js:74:21)
    at getParamsAndProps (project_path/packages/client/node_modules/astro/dist/core/render/params-and-props.js:19:29)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async matchRoute (project_path/packages/client/node_modules/astro/dist/vite-plugin-astro-server/route.js:35:7)
    at async run (project_path/packages/client/node_modules/astro/dist/vite-plugin-astro-server/request.js:52:28)
    at async runWithErrorHandling (project_path/astro-boilerplate/packages/client/node_modules/astro/dist/vite-plugin-astro-server/controller.js:65:5)
    at async handleRequest (project_path/packages/client/node_modules/astro/dist/vite-plugin-astro-server/request.js:48:3)

This bug seems to have been introduced in the update from version 2.8.4 to 2.8.5.

It is possible to reach the slugged page in version 2.8.4, although the id will not be present in Astro.params, which will return an empty object instead of and object containing the id and value for the slug.

Please see the repo on Stackblitz and click the link on the home page to see the error.

What's the expected result?

I would expect to be able to navigate to urls that have been encoded using encodeURI() or encodeURIComponent(). I also expect these url params to be present in Astro.params.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-mt9ply

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Aug 4, 2023
@svensigmond svensigmond changed the title Routing error when navigation to path with encoded forward slash Routing error when navigating to path that includes an encoded forward slash Aug 4, 2023
@natemoo-re natemoo-re added the - P3: minor bug An edge case that only affects very specific usage (priority) label Aug 7, 2023
@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Aug 7, 2023
@rishi-raj-jain
Copy link
Contributor

Shouldn't you be using [...slug].astro if you want to go nested paths?

@rishi-raj-jain
Copy link
Contributor

@svensigmond Did you get a look at my comment?

@svensigmond
Copy link
Author

Hello @rishi-raj-jain, I had missed your comment. We resolved the issue internally by changing the value of the id in the url, but I did give your solution a spin in the stackblitz example. Using [...id].astro instead of [id].astro does indeed fix the problem but if I correctly understand the documentation for rest parameters this is because Astro now treats 'EUR/USD' as a single parameter. Even though this works in this case, I do feel that I should still be able to encode a url with a forward slash without Astro throwing an exception.

For example lets say I would like to go to a url like this: http://example.com/currencies/eur%2Fusd/details. This would not be possible if I understand the rest parameter slugs in Astro correctly, because it would treat everything after currencies as eur/usd/details even though no hierarchical structer exists between eur and usd. only between eur/usd and details.

To summarize, yes using the rest parameters in the slug does fix the stack blitz example, but merely hides the fact that using encoded forward slashes with Astro is broken. (unless this is by design and I missed this information in the documentation).

@yuvalsade
Copy link

any news on this one?

@ematipico
Copy link
Member

The expectation is a bit unclear to me. What should be the value id in your case, @svensigmond?

@ematipico ematipico added the needs response Issue needs response from OP label Jan 4, 2024
@yuvalsade
Copy link

looks like it should be 'eur/usd' inside Astro.params. as it is "eur%2Fusd" in the URL. encountered the same issue myself, with diffrent types of values.

@svensigmond
Copy link
Author

svensigmond commented Jan 4, 2024

@yuvalsade is correct @ematipico. I wanted to use eur/usd as the id, but encoded with encodeURIComponent(). Here is a snippet from the Stackblitz example: (src/pages/index.astro)

<a href={/currencies/${encodeURIComponent('eur/usd')}}>View currency detail page for EUR/USD

@ematipico ematipico removed the needs response Issue needs response from OP label Jan 12, 2024
@asdfjkalsdfla
Copy link

I'm also experiencing this one.

@ascorbic
Copy link
Contributor

ascorbic commented Oct 2, 2024

This may be fixed by #12079

@florian-lefebvre
Copy link
Member

The PR does not fix it

@florian-lefebvre
Copy link
Member

We do not plan to support this usecase. Here, looks like you need 2 params instead, that could be /[from]/[to] or /[from]- [to]

@florian-lefebvre florian-lefebvre closed this as not planned Won't fix, can't repro, duplicate, stale Oct 7, 2024
@yuvalsade
Copy link

yuvalsade commented Oct 7, 2024 via email

@ematipico ematipico reopened this Oct 9, 2024
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Oct 9, 2024
@ematipico
Copy link
Member

I'm glad to bring some good news. This will be supported in Astro v5. You can check it out in 5.0.0-beta.4. Here's a stackblitz as proof:

https://stackblitz.com/edit/github-mt9ply-2xtkfc?file=astro.config.mjs,package.json,src%2Fpages%2Fcurrencies%2F[currency].astro

@svensigmond
Copy link
Author

hi @ematipico, I checked out the stackblitz example you linked and I still get an error when navigating to the page with the link on the index page:

image

It is slightly different from the error in my original stackblitz example, but an error nonetheless:

Original error:
image

@ematipico
Copy link
Member

@svensigmond I suggest providing a reproduction. The initial reproduction uses a very outdated version of Astro

@svensigmond
Copy link
Author

Hi @ematipico I have taken another look at the repo you posted at Oct. 9.

There seems to be an error in /pages/currency/[id].astro.

At lines 9, 15 en 18 we are destructuring / using currency. Changing this to id does indeed prove the error has been fixed in Astro 5 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 beta Related to the Astro 5.0 beta - P3: minor bug An edge case that only affects very specific usage (priority) needs triage Issue needs to be triaged
Projects
None yet
Development

No branches or pull requests

8 participants