fix(routing): partially dynamic segments are being truncated #10379
Conversation
🦋 Changeset detectedLatest commit: 402050e The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
pkg: astro
|
I reviewed it from my phone, so it was just a quick look over. I'll review it more carefully later today. Thanks for the PR! |
.changeset/five-colts-rescue.md
Outdated
| "astro": patch | ||
| --- | ||
|
|
||
| Fix the issue of partially truncated dynamic segments. |
There was a problem hiding this comment.
| Fix the issue of partially truncated dynamic segments. | |
| Fixes a routing issue with partially truncated dynamic segments. |
Would be great to add an example as well maybe?
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
| const [{ dynamic, content }] = segment; | ||
|
|
||
| return segment.length > 1 | ||
| ? segment.map((s) => d([s])).join('') |
There was a problem hiding this comment.
Could you confirm I have got this right?
This one line is the fix.
Previously, we ignored all but the first RoutePart.
Now, each RoutePart gets passed to this d() function. And the function ultimately still wraps the dynamic RouteParts in square brackets same as it was done before.
There was a problem hiding this comment.
Sure thing, you've got it right.
Would there be any issues with this?
There was a problem hiding this comment.
No, this is great - just took me a while to understand the recursive map, so I thought I'd double check.
There was a problem hiding this comment.
I think the recursive mapping is more confusing than needed and either two normal for loops or two nested maps would make it easier to comprehend.
There is one test check that was removed instead of updated.
Besides that, looking good! Sorry for the delay in the review
Changes
fix #10122
Testing
units/routing/manifest.test.js
Docs