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

[routing] Replace consumer with SAM interface with receiver #2049

Conversation

dzikoysk
Copy link
Member

@dzikoysk dzikoysk commented Nov 10, 2023

Basically, instead of:

cfg.router.mount(Dsl) { dsl ->
    dsl.before("/abc") {} 
}

We'll be using

cfg.router.mount(Dsl) {
    before("/abc") {} // based on `this`
}

No changes for Java users.

@tipsy
Copy link
Member

tipsy commented Nov 11, 2023

So this will look identical to using the Apibuilder from Kotlin.. And we'll introduce a split in syntax between Java and Kotlin..

Why not use the Apibuilder instead ?

@dzikoysk
Copy link
Member Author

We could also provide SAM with receiver as a second mount variant 🤔

I don't like ApiBuilder (static imports, context & building routes via nested groups) + that's simply a bit different approach. Also, I'm mainly looking at it from the perspective of 4 other routing implementations as I'm in a process of porting all of them to the new API.

@zugazagoitia
Copy link
Member

Can you add some Java API compile tests? In case some of us touch this and mess it up.

@dzikoysk
Copy link
Member Author

Actually, there are some tests in Java that use this API, but like I said, this change does not affect it as this SAM interface with receiver looks basically the same as Consumer in Java. E.g. this one:

config.router.mount(router -> {
router.beforeMatched(ctx -> {
Set<RouteRole> routeRoles = ctx.routeRoles();
String userRole = ctx.queryParam("role");
if (userRole == null || !routeRoles.contains(JRole.valueOf(userRole))) {
throw new UnauthorizedResponse();
}
});
});

@tipsy
Copy link
Member

tipsy commented Nov 15, 2023

This still introduces a split in API for Java and Kotlin users, which is something we've been trying very hard to avoid so far in Javalin. The fact that it looks like the statically imported ApiBuilder is also a major downside IMO. We need to find some sort of solution that both looks and works the same from both languages, or it needs to live in an extension artifact.

@dzikoysk
Copy link
Member Author

dzikoysk commented Nov 16, 2023

Because this feature is only dedicated for Kotlin users, I've replaced it with an extension function. Thanks to that, default mount function work the same as in Java, but there's still an opt-in possibility to use a DSL in kt - that's mainly useful in 3rd party routing impls.

Quick note: I had to use a hidden functionality of kotlin std (@LowPriorityInOverloadResolution) to prioritize imported extension function to avoid signatures clash. (it'd be highlighted in the IDE and may stop working in the future - this tells compiler what to do in that case and removes the warning)

@tipsy
Copy link
Member

tipsy commented Nov 16, 2023

Making it an extension function doesn't really solve the main issue though, you still have something that looks and acts like the ApiBuilder, but without the static imports and path(). The potential for confusion here is quite high, and the gain seems to be very low (this vs named param).

@dzikoysk
Copy link
Member Author

dzikoysk commented Nov 16, 2023

I can define this extension function on my side, but I still need @LowPriorityInOverloadResolution on the core mount method. Getting rid of this vs named param is a big part of the dsl (api is clean and does not bring unnecessary components that don't provide any value in given context). Using multiple nested unnamed (it) parameters is kind of an antipattern, but named params would introduce a lot of unnecessary noise, so both solutions are kinda a lose-lose situation.

@dzikoysk
Copy link
Member Author

So now this improvement is possible on my side, but it's not affecting public API in Javalin core. Do you think you're fine with this change at this point? 🤔

@tipsy
Copy link
Member

tipsy commented Nov 18, 2023

So now this improvement is possible on my side, but it's not affecting public API in Javalin core. Do you think you're fine with this change at this point? 🤔

Yes, thank you 🙏

@tipsy
Copy link
Member

tipsy commented Nov 18, 2023

invokeWithAsSamWithReceiver is an odd name, but I can change it after merging 😁

@tipsy tipsy merged commit 9d6de36 into javalin:master Nov 18, 2023
9 checks passed
@dzikoysk dzikoysk deleted the replace-consumer-with-sam-with-receiver branch November 18, 2023 12:46
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.

None yet

3 participants