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: making non-dynamic routes take precedence over dynamic ones #668

Closed
wants to merge 1 commit into from
Closed

fix: making non-dynamic routes take precedence over dynamic ones #668

wants to merge 1 commit into from

Conversation

erickzanardo
Copy link
Contributor

Status

READY

Description

Fixes #658
Fixes #631

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@erickzanardo erickzanardo marked this pull request as draft May 29, 2023 14:43
erickzanardo
erickzanardo commented May 29, 2023
View reviewed changes
bricks/dart_frog_dev_server/hooks/pre_gen.dart
'directories': configuration.directories
.map((c) => c.toJson())
.toList()
.reversed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felangel do you remember why this reversed call was in here?

If I kept it, it would "nullify" the ordering set to make that non dynamic routes takes precedence.

I tested without the reversed call and it seems to be working fine, but I believe that this call must have had a reason to be in here before, maybe you remember and could advise me which cases it covers so I can test with the new changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

iirc the idea was to order the directories from most nested to least nested (most specific to least specific) so that the router generation would mirror that order and routes like:

/users/[id]/pets

Would be registered and handled before routes like:

/users/[id]

@erickzanardo
Copy link
Contributor Author

Closing in favor of #687

@erickzanardo erickzanardo deleted the fix/routes-mount-order branch June 6, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felangel felangel Awaiting requested review from felangel

@renancaraujo renancaraujo Awaiting requested review from renancaraujo

@wolfenrain wolfenrain Awaiting requested review from wolfenrain

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Mount non-dynamic routes first fix: dynamic routes override non dynamic ones.
2 participants
@erickzanardo @felangel