Skip to content

fix: redirect named splat filesystem routes correctly for SSR/DSG pages #120

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

Merged
merged 3 commits into from
Apr 27, 2022

Conversation

orinokai
Copy link
Contributor

The plugin redirects SSR/DSG pages to the relevant handler function, but was failing for SSR/DSG pages using filesystem routing named splats, because the param name was being incorrectly output in the fromPath of the redirect.

This PR ensures that redirects contains only the splat and the named param is only forwarded to the handler function.

In addition, this PR ensures that SSG client-only routes aren't also erroneously output alongside SSR/DSG redirects when using file system routing.

@orinokai orinokai added the type: bug code to address defects in shipped code label Apr 21, 2022
@orinokai orinokai requested a review from a team April 21, 2022 16:35
@orinokai orinokai self-assigned this Apr 21, 2022
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Great! Glad you worked it out. Presumably this doesn't change any tests because we don't have filesystem routes in the examples?

[pebble] it would be good to add filesystem route tests

Copy link
Contributor

@ericapisani ericapisani left a comment

Choose a reason for hiding this comment

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

This looks great!

If you have the time, adding test coverage on this would be stellar. (edit: just noticing Matt's initial review above and am +1-ing it)

There's an existing test that focuses on the SSR/DSG redirects functionality that could be used as a helpful resource.

@kodiakhq kodiakhq bot merged commit cebe8d8 into main Apr 27, 2022
@kodiakhq kodiakhq bot deleted the rs/file-routing branch April 27, 2022 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants