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

add dev checks for plugin route conflicts #1558

Open
deer opened this issue Aug 1, 2023 · 10 comments · May be fixed by #1568
Open

add dev checks for plugin route conflicts #1558

deer opened this issue Aug 1, 2023 · 10 comments · May be fixed by #1568

Comments

@deer
Copy link
Contributor

deer commented Aug 1, 2023

We should add two checks here:

  1. routes within a plugin conflicting are conflicting with each other. The plugin author has done something really silly! (Maybe this isn't even necessary?)
  2. routes across plugins conflicting with each other, or with the user's routes. This is very possible to happen.

from: #1545 (comment)
@mbhrznr, any interest in picking this up? 🤞

@mbhrznr
Copy link
Contributor

mbhrznr commented Aug 1, 2023

count me in! 💪

@thegaryroberts
Copy link

On point 1: When starting out I got caught out a lot with routes conflicts when having dynamic routes at the same directory level as named routes... plugin authors might make the same mistake. Also it would be good to validate this for routes in general (if this doesn't happen already).

Or at minimum the docs here could do with a note about this gotcha: https://fresh.deno.dev/docs/concepts/routing

@mbhrznr
Copy link
Contributor

mbhrznr commented Aug 22, 2023

was finally able to pick this up:
given that allRoutes in our context already contains the manifest's routes as well that all of the plugin routes, we already should have some base coverage on the conflicts w/ #1568.

however as mentioned by @thegaryroberts i'd invest some time in checking dynamic route conflicts.
currently we'd only have checks for strict equally on the patterns, yet something like /products/:id and /products/:article wouldn't be picked up.

@mbhrznr
Copy link
Contributor

mbhrznr commented Aug 25, 2023

as expected the checks were also applicable for plugins ootb.
added a dedicated check for dynamic routes to #1568.

@mbhrznr
Copy link
Contributor

mbhrznr commented Aug 26, 2023

was scanning the still open prs, wouldn't this be tackled by #1414 anyway?
@deer if you want to i can migrate that into #1568 as well, seems to be way more robust than my current approach.

@deer
Copy link
Contributor Author

deer commented Aug 29, 2023

I must admit I haven't checked whether what I've done for 1414 covers routes added by plugins. But as I'm reading it over, I guess the approach would work, since it checks after routes have been loaded. How do you want to proceed?

@mbhrznr
Copy link
Contributor

mbhrznr commented Aug 29, 2023

if you don't mind, i'd migrate #1414's changes into #1568 to bundle all of the dev checks under one umbrella pr.
however we might want to add some further meta information about the type of routes, as routes from plugins get the ./routes* prefix via getRoutesFromPlugins.

thus the warning would always contain ./routes/<pattern> even though the warning originated from a plugin route.


for now i'd also set #1568 back to a draft state.

@deer
Copy link
Contributor Author

deer commented Aug 30, 2023

Ok, so I'll close #1414 and you can update #1568 so that it closes this issue and #475. Sound good?

@mbhrznr
Copy link
Contributor

mbhrznr commented Aug 30, 2023

sounds like a plan! thank you!

@deer
Copy link
Contributor Author

deer commented Aug 30, 2023

And don't forget to get 539 closed as well!

@mbhrznr mbhrznr linked a pull request Sep 4, 2023 that will close this issue
12 tasks
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 a pull request may close this issue.

3 participants