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

Routing Groups #1398

Closed
MaximBazarov opened this Issue Dec 30, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@MaximBazarov
Copy link
Contributor

MaximBazarov commented Dec 30, 2017

After lots of discussions we produced the following functions:

Create new group

let users = router.group("user")
users.get("auth", use: userAuthHandler)
users.get("profile", use: userProfileHandler)

I named it group instead of grouped because function returns new, just created, group rather than modified router

Create a new group and configure it in-place

router.grouped(with: "user") { group in
    group.get("auth", use: userAuthHandler)
    group.get("profile", use: userProfileHandler)
}

Here grouped does make sense because function modifies the router

Using middleware (old-fashioned)

let users = router.group("user").using(SomeMiddleware())
users.get("auth", use: userAuthHandler)

In this case using makes sense for both cases

Using middleware (old-fashioned) and configure a group which use this middleware in-place

router.using(AuthorizationMiddleware()) { group in
    group.get("profile", use: userProfileHandler)
}

Using function as a middleware

Swift is a functional language, and I think that is the best to use this style naturally. Instead of caring the responsibility to store references to a middleware instances it is possible to simply store pure functions which apply to requests

let users = router.group("user").using({ req, next in
    return try userService.authorized(user)
})
users.get("auth", use: userAuthHandler)

Using a function as a middleware and configure it in-place

func userMustBeAuthorized(request: Request, next: Responder) throws -> Future<Response> {
    return try userService.authorized(user)
}

router.using(userMustBeAuthorized) { group in
    group.get("profile", use: userProfileHandler)
}
@MaximBazarov

This comment has been minimized.

Copy link
Contributor Author

MaximBazarov commented Dec 30, 2017

Guys please let's create a perfect naming here

@Joannis

This comment has been minimized.

Copy link
Member

Joannis commented Jan 5, 2018

I think we should look into Middlewares more, specifically regarding thread safety. If we inject a route within a middleware (as with Vapor 3 currently, and this proposal), accessing instance variables can crash. I so like the using (and other) naming, although using with a trailing closure is an awesome but confusingly named helper to me. I'm not using a closure, I'm using the closure as a middleware that maps a Request.

@MaximBazarov

This comment has been minimized.

Copy link
Contributor Author

MaximBazarov commented Jan 5, 2018

we can also remove the using(_ middleware: Middleware..., functions and only keep using(_ respond: @escaping MiddlewareFunction.Respond functions. We'll still be able to use old middleware just passing Middleware.respond function:

// register in services or somewhere
let someMiddleware = SomeMiddleware()

//...
router.using(someMiddleware.respond) { group in
    group.get("profile", use: userProfileHandler)
}

And downside is: we need to warn user to not use approach with in-place instantiation:

router.using(SomeMiddleware().respond) { group in
    group.get("profile", use: userProfileHandler)
}

but better to do it once and change the docs than having issues with threads all the time

@0xTim

This comment has been minimized.

Copy link
Member

0xTim commented Jan 5, 2018

I don't like that lack of safety that this would give you - warning users not to use it isn't really ideal if it is going to crash the application, especially given it looks like the same thing and compiles without any warnings etc

@MaximBazarov

This comment has been minimized.

Copy link
Contributor Author

MaximBazarov commented Jan 8, 2018

@0xTim I totally agree with you, but in this case it's impossible to move on and keep it compile time safe

@tanner0101 tanner0101 added this to the 3.0.0 milestone Jan 23, 2018

@tanner0101 tanner0101 self-assigned this Jan 23, 2018

@tanner0101 tanner0101 added this to Backlog in Vapor 3 Feb 12, 2018

@MaximBazarov

This comment has been minimized.

Copy link
Contributor Author

MaximBazarov commented Feb 20, 2018

Merged

@tanner0101 tanner0101 moved this from Backlog to Done in Vapor 3 Feb 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment