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

Fix combine bug #461

Merged
merged 3 commits into from
Feb 8, 2025
Merged

Fix combine bug #461

merged 3 commits into from
Feb 8, 2025

Conversation

stackoverfloweth
Copy link
Contributor

when we unified our param utilities into withParams, we also changed the logic for the combine utilities like combinePath, combineQuery, etc. Those utilities used to just spread the params together

return {
    value: newPathString,
    params: { ...parentPath.params, ...childPath.params },
}

But with the half thought of a toString() method I updated this to re-call withParams

return withParams(newPathString, { ...parentPath.params, ...childPath.params })

Even without the toString() property this felt right but caused a bug. Because withParams renames param keys to possibly start with a leading "?" it ends up being lost when we re-call withParams when combining. Therefore if you assign a custom param (anything other than String) to a route with a parent, it will just become a string param.

For this PR I decided to update the getParam utility to check for a param without "?", then check for the param with a leading "?", then use the default "String".

Alternatively I'd be happy to go back to the version we had previously where we do not re-call withParams.

@stackoverfloweth stackoverfloweth self-assigned this Feb 8, 2025
Copy link

netlify bot commented Feb 8, 2025

Deploy Preview for kitbag-router ready!

Name Link
🔨 Latest commit 90c51f4
🔍 Latest deploy log https://app.netlify.com/sites/kitbag-router/deploys/67a6c9e9aa842f0008e47ced
😎 Deploy Preview https://deploy-preview-461--kitbag-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@stackoverfloweth stackoverfloweth merged commit 75cab74 into main Feb 8, 2025
6 checks passed
@stackoverfloweth stackoverfloweth deleted the fix-combine-bug branch February 8, 2025 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants