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.

Sign up for GitHub

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

Jump to bottom

fix: a bug #6330

Closed
wants to merge 1 commit into from
Closed

fix: a bug #6330

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/qwik-city/runtime/src/route-matcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ function recursiveScan(
}
let pathIdx = pathLength;
const sep = suffix + '/';
let depthWatchdog = 5;
let depthWatchdog = 8;
Copy link
Member

Choose a reason for hiding this comment

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

I think you can set this to 50 and also add a console.error before line 160 when it gets to 0

Copy link
Contributor Author

@JerryWu1234 JerryWu1234 May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

what do you think about the thrown exception?

Copy link
Member

Choose a reason for hiding this comment

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

we merged #6327 which remove the limit and adds a test. do we want a limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do I close this pr?

Copy link
Member

Choose a reason for hiding this comment

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

I wanted to get his pr in since he was first-time contributor 😁 I think we should have a limit of 50 or some way to set a limit. I don't think any app will even reach 50

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PatrickJS still needs this limit？So do i continue this PR ?

Copy link
Member

Choose a reason for hiding this comment

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

@JerryWu1234 I'll close the PR I know you're working on a few other things 🙏 thanks so much for helping us

while (pathIdx >= pathStart && depthWatchdog--) {
const match = matchRoutePart(route, routeStart, routeLength, path, pathIdx, pathLength);
if (match) {
Expand Down
Loading