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: 404 Cannot find any route matching, when adding a child route. #103

Closed
wants to merge 8 commits into from

Conversation

Aslemammad
Copy link
Contributor

Resolves nitrojs/nitro#2419

The maxDepth was more than remaining in some cases. When server/routes/[owner]/[repo]/[packageAndRefOrSha].get.ts and server/routes/[owner]/[repo]/[npmOrg]/[packageAndRefOrSha].get.ts are present.

The latter would affect the maxDepth, so in such cases, there might be children with lower maxDepth that are candidate. But === did not work for those cases, so let's consider those with more maxDepth anyway.

src/router.ts Outdated
? 0
: a.maxDepth === remaining
? -1
: b.maxDepth === remaining
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those with the exact remaining have priority anyway.

@pi0
Copy link
Member

pi0 commented May 10, 2024

Would you please add regression test too? (that fails without this fix)

@Aslemammad
Copy link
Contributor Author

Would you please add regression test too? (that fails without this fix)

Sure, I'll do it tomorrow morning! is that ok?

testRouter(
[
"/",
"/:packageAndRefOrSha",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one triggers the if (node && node.placeholderChildren.length > 1) { condition.

@Aslemammad
Copy link
Contributor Author

@pi0 Here you go! Let me know what you think, I brought the exact test cases we have in https://github.com/stackblitz-labs/pkg.pr.new 😅

@Aslemammad Aslemammad closed this May 25, 2024
@pi0 pi0 reopened this May 27, 2024
@pi0
Copy link
Member

pi0 commented May 27, 2024

(keeping draft open for test reference)

@pi0 pi0 marked this pull request as draft May 27, 2024 08:41
Copy link

codecov bot commented May 27, 2024

Codecov Report

Attention: Patch coverage is 95.65217% with 1 lines in your changes are missing coverage. Please review.

Please upload report for BASE (v1@9157457). Learn more about missing BASE report.

Files Patch % Lines
src/router.ts 95.65% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##             v1     #103   +/-   ##
=====================================
  Coverage      ?   70.14%           
=====================================
  Files         ?        8           
  Lines         ?      633           
  Branches      ?       95           
=====================================
  Hits          ?      444           
  Misses        ?      185           
  Partials      ?        4           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pi0 added a commit that referenced this pull request Jul 3, 2024
Co-Authored-By: Mohammad Bagher Abiyat <zorofight94@gmail.com>
@pi0
Copy link
Member

pi0 commented Jul 3, 2024

Hey. I'm back to radix3! Added your nice tests via 1747ff7 to main (v2) branch as first step. Will try to investigate more.

@pi0 pi0 mentioned this pull request Jul 3, 2024
@Aslemammad
Copy link
Contributor Author

Thank you so much! ❤️

@pi0
Copy link
Member

pi0 commented Jul 4, 2024

Should be fixed after. #107

@pi0 pi0 closed this Jul 4, 2024
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.

404 Cannot find any route matching, when adding a child route.
2 participants