-
Notifications
You must be signed in to change notification settings - Fork 12
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: segment parsing #43
base: main
Are you sure you want to change the base?
Conversation
@sandulat can you please take a look at this PR? |
5757fff
to
99de449
Compare
@lifeiscontent Hey sorry, I've missed the notification. I'll take a look tomorrow. Thanks a lot for the PR! |
@lifeiscontent can't this be solved with this simpler approach? export function route<T extends string>(
path: T,
params?: Record<string, any>
): T {
if (params) {
const segments = path.split(/\/+/).map((segment) => {
if (segment.startsWith(":")) {
const keySegments = segment
.replace(":", "")
.replace("?", "")
.split(/(?=\.\w+)/);
if (keySegments[0] in params) {
return `${params[keySegments[0]]}${keySegments.slice(1).join("")}`;
}
if (segment.endsWith("?")) {
return null;
}
}
return segment;
});
return segments.filter((value) => value !== null).join("/") as T;
}
return path;
} Can you give it a try? |
@sandulat re: can't this be solved with this simpler approach? got this from test harness:
|
6d212f5
to
3a18129
Compare
93ffc83
to
6f16ad4
Compare
@sandulat let me know if you need anything from me, the routes.d.ts file generation might need to be updated for the new param but I'm having a bit of trouble reading that code. |
Hey @sandulat friendly bump on this 😅 |
I've simplified the algorithm to a single loop by reconstructing the path in reverse order. |
@sandulat can you please take a look at this? |
Hey @lifeiscontent, I'm really sorry for the delay. I've been overloaded with work. Regarding optional segments, I believe that instead of expanding the runtime responsibility of the This means that for a route like
This would result in the following usage:
So, in summary:
export function route<T extends string>(
path: T,
params?: Record<string, any>
): T {
if (params) {
return path
.split(/\/+/)
.map((segment) => {
if (segment.startsWith(":")) {
const keySegments = segment.replace(":", "").split(/(?=\.\w+)/);
if (keySegments[0] in params) {
return `${params[keySegments[0]]}${keySegments.slice(1).join("")}`;
}
}
return segment;
})
.join("/") as T;
}
return path;
} |
@sandulat I think this is ready for review again, for the escaped segments, I think we can open a separate PR to fix, as the types for routes.d.ts need to be updated to include |
020d9d9
to
b63a7f0
Compare
the routes function had a bug I found with optional segments that aren't parameterized.