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

feat: nested routers initial pass #73

Merged
merged 7 commits into from
Oct 11, 2024
Merged

feat: nested routers initial pass #73

merged 7 commits into from
Oct 11, 2024

Conversation

JohnCoene
Copy link
Collaborator

Allows nesting routers.

Instead of grabbing the routes, receivers (websocket handlers), and middlewares when the router is passed to use we delay that execution until start where we recursively go through all 3 items. Doing so recursively means we can nest as many routers as we want.

@kennedymwavu probably needs more testing, I haven't tested middlewares but this should work too. It's useful if, for instance, you have a specific middleware to check for authentication that you only want to apply to a subset of routes that are protected, e.g.: homepage will likely not require authentication while all routes in /app may require auth.

library(htmltools)
library(ambiorix)

app <- Ambiorix$new()

user_router <- Router$new("/user")
profile_router <- Router$new("/profile")

# /user/profile
user_router$use(profile_router)

app$use(user_router)

user_router$get("/", \(req, res) {
  res$send(h1("Users"))
})

profile_router$get("/", \(req, res) {
  res$send(h1("The user"))
})

profile_router$get("/me", \(req, res) {
  res$send(h1("Me me me"))
})

app$get("/", \(req, res) {
  page <- div(
    tags$a(href = "/user", "User"),
    tags$a(href = "/user/profile", "Profile"),
    tags$a(href = "/user/profile/me", "Me")
  )
  res$send(page)
})

app$start()

@JohnCoene JohnCoene added the feature New feature label Oct 9, 2024
@JohnCoene JohnCoene self-assigned this Oct 9, 2024
@kennedymwavu
Copy link
Contributor

kennedymwavu commented Oct 9, 2024

this is really great!
but now the existing functionality of middleware is broken :/
here's a small reprex:

devtools::load_all()

mdd <- \(req, res) {
  print("inside a middleware")
  forward()
}

home_get <- \(req, res) {
  res$send("Ambiorix")
}

Ambiorix$
  new(port = 8000L)$
  use(mdd)$
  get("/", home_get)$
  start()

this is the error i get on the console:

ℹ Loading ambiorix
Error in super$get_middleware() : object 'router' not found
Calls: <Anonymous> -> <Anonymous>
Execution halted

hint:

looks like there are two small bugs...

      for(receiver in private$.receivers) {
        receivers <- router$get_receivers(receivers)
      }
      for(middleware in private$.middleware) {
        middlewares <- router$get_middleware(middlewares)
      }

@JohnCoene
Copy link
Collaborator Author

Right, now we're good.

library(htmltools)
library(ambiorix)

app <- Ambiorix$new()

# routers
first_router <- Router$new("/first")
second_router <- Router$new("/second")
third_router <- Router$new("/third")

# middleware
second_middleware <- \(req, res) {
  cat("Hello from /second...\n")
}

# nest
second_router$use(second_middleware)
second_router$use(third_router)
first_router$use(second_router)

app$use(first_router)

first_router$get("/", \(req, res) {
  res$send(h1("Users"))
})

second_router$get("/", \(req, res) {
  res$send(h1("Second!"))
})

third_router$get("/", \(req, res) {
  res$send(h1("Third!"))
})

app$get("/", \(req, res) {
  page <- div(
    tags$a(href = "/first", "1"),
    tags$a(href = "/first/second", "2"),
    tags$a(href = "/first/second/third", "3")
  )
  res$send(page)
})

app$start()

@jrosell
Copy link

jrosell commented Oct 10, 2024

That't cool. One question, should we add an issue for adding route helpers similarly like ruby and rails routers have with support for nested routes?

For example, in the case of resources :photos:

photos_path returns /photos

new_photo_path returns /photos/new

edit_photo_path(:id) returns /photos/:id/edit (for instance, edit_photo_path(10) returns /photos/10/edit)

photo_path(:id) returns /photos/:id (for instance, photo_path(10) returns /photos/10)

How could be for profile paths?

@kennedymwavu
Copy link
Contributor

Right, now we're good.

@JohnCoene good progress, the nesting is now working well. but...

if we have a middleware set for the second router, it should not run when endpoints of the first router are accessed.
using your reprex:

  • GET on /: works as expected, does not call second_middleware()
  • GET on /first: calls second_middleware(), shouldn't happen
  • GET on /first/second: calls second_middleware(), as expected

here's the console output:

ℹ 10-10-2024 10:17:24 GET on /
Hello from /second...
ℹ 10-10-2024 10:17:45 GET on /first
Hello from /second...
ℹ 10-10-2024 10:18:39 GET on /first/second

@JohnCoene
Copy link
Collaborator Author

@kennedymwavu I'll check it should indeed not work like that but I was sure I got this right, I don't remember seeing that, I probably did something wrong.

@jrosell we support parameters in ambiorix so you can totally do that, I'll share an example, unless I misunderstand your question.

@JohnCoene
Copy link
Collaborator Author

I fixed it, though it breaks all the tests... (it seems to be an issue with testthat to be honest, I'm getting some strange error).

And here's an example + for @jrosell using the parameters, try /first/second/third/fourth/something

app <- Ambiorix$new()

# routers
first_router <- Router$new("/first")
second_router <- Router$new("/second")
third_router <- Router$new("/third")
fourth_router <- Router$new("/fourth")

# middleware
second_middleware <- \(req, res) {
  cat("Hello from /second...\n")
}

# middleware
second_router$use(second_middleware)

# routers
third_router$use(fourth_router)
second_router$use(third_router)
first_router$use(second_router)
app$use(first_router)

first_router$get("/", \(req, res) {
  res$send(h1("First"))
})

second_router$get("/", \(req, res) {
  res$send(h1("Second!"))
})

third_router$get("/", \(req, res) {
  res$send(h1("Third!"))
})

fourth_router$get("/", \(req, res) {
  res$send(h1("fourth"))
})

fourth_router$get("/:this", \(req, res) {
  res$send(h1("path:", req$params$this))
})

app$get("/", \(req, res) {
  page <- div(
    tags$a(href = "/first", "1"),
    tags$a(href = "/first/second", "2"),
    tags$a(href = "/first/second/third", "3"),
    tags$a(href = "/first/second/third/fourth", "4")
  )
  res$send(page)
})

app$start()

@jrosell
Copy link

jrosell commented Oct 10, 2024

Sorry for not explaining it well. I'm talking about some path helper like:

For example some get_path(second_router) returning "/first/second" so it can be used in the links.

@JohnCoene
Copy link
Collaborator Author

Ah I get it!

I don't think you explained it poorly, I work on OSS in the wee hours of the night so I tend to misread.

We could totally consider that, I haven't explored rails but have only heard good things and DHH surely makes me want to try, I should take a look at rails to see if there are ideas/features we could port to Ambiorix.

I'm just wondering where you use those helpers in your code? Would you have an example or pseudo code for me to understand?

@jrosell
Copy link

jrosell commented Oct 10, 2024

For unnested routers, could be:

  • path(photos) returns /photos
  • new_path(photos) returns /photos/new
  • edit_path(photos, id) returns /photos/:id/edit (for instance, edit_path(photos, 10) returns /photos/10/edit)
  • path(photos, id) returns /photos/:id (for instance, path(photos, 10) returns /photos/10)

Then you could have glue('<a href="{path(photos)}">List all</a> <a href="{new_path(photos)}">New</a> <a href="{edit_path(photos, 1)">Edit it</a> <a href="{path(photos, 1)}">Show it</a>')

Why is that useful? When you nest the routers, you don't have to change the HTML with ne new routes, the helpers make it right for you.

Rails priorizes convention over configuration, and I think it's wonderful principle (model name = url path = db table)

Hope it makes sense.

@JohnCoene
Copy link
Collaborator Author

Ah, I see!

Not sure how we can make this work but we can certainly look into it. It pertains to the rendering engine which I was just talking about with @kennedymwavu needs a good revamp.

@kennedymwavu
Copy link
Contributor

I fixed it, though it breaks all the tests... (it seems to be an issue with testthat to be honest, I'm getting some strange error).

@JohnCoene what error are you getting? all tests are passing on my end, albeit with some warnings (see attached).

image

@JohnCoene
Copy link
Collaborator Author

I had one failure which I just fixed, not sure what I did before

Copy link
Contributor

@kennedymwavu kennedymwavu left a comment

Choose a reason for hiding this comment

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

lgtm!

@JohnCoene JohnCoene merged commit 6e856d9 into master Oct 11, 2024
5 checks passed
@JohnCoene JohnCoene deleted the feat/nested-routers branch October 11, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants