-
Notifications
You must be signed in to change notification settings - Fork 35
Add webMain support #145
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
base: main
Are you sure you want to change the base?
Add webMain support #145
Conversation
kalinjul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a quick look, didn't have time to test it yet.
Would the sample app work with this?
| val result = webFlow.startWebFlow(request.url, request.url.parameters["redirect_uri"].orEmpty()) | ||
|
|
||
| if (result !is WebAuthenticationFlowResult.Success) { | ||
| // browser closed, no redirect | ||
| Result.failure(OpenIdConnectException.AuthenticationCancelled()) | ||
| return Result.failure(OpenIdConnectException.AuthenticationCancelled()) | ||
| } | ||
|
|
||
| if (result.responseUri == null) { | ||
| return Result.failure(OpenIdConnectException.AuthenticationFailure("No Uri in callback from browser.")) | ||
| } | ||
|
|
||
| return when (val error = getErrorResult<AuthCodeResult>(result.responseUri)) { | ||
| null -> { | ||
| val state = result.responseUri.parameters["state"] | ||
| val code = result.responseUri.parameters["code"] | ||
| Result.success(AuthCodeResult(code, state)) | ||
| } | ||
| else -> { | ||
| return error | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep this in the old-style nested form so it looks just like in iOS/Android/JVM?
I'd like to refactor out this code block anyways, until then i'd like to keep it structurally equal :)
| add(project.projectDir.path) | ||
| add(project.projectDir.path + "/commonMain/") | ||
| add(project.projectDir.path + "/wasmJsMain/") | ||
| add(project.projectDir.path + "/webMain/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need both webMain and wasmJsMain here?
| js(IR) { | ||
| browser() | ||
| binaries.library() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have this in a "configureJsTarget" function just like the others?
This is a PR for the JS issue.
Description of your changes
WebPopupFlowan expect class to allow for JS and wasmJS implementation.WebMainCodeAuthFlowFactorywhich is a common factory for both JS and wasmJS targets.How has this been tested?
I tested using a Compose Multiplatform, where I used it to open and login via Keycloak Oidc.
Initially, I tested with "empty" implementations of the JS actual classes. I built my app with
composeApp:jsBrowserDevelopmentRunIt failed to do anything as expected. I then implemented the JS actual classes, and it worked.Note that even Compose Web with JS fallback still uses wasm for the Skia UI. I have yet to test it with a pure Kotlin/JS implementation (such as Kotlin React), but I do not have much experience with Kotlin/JS.
Is this a (API-) breaking change?
It breaks an experimental API. Namely,
WasmCodeAuthFlowFactorywas renamed toWebMainCodeAuthFlowFactory.Users would have to update
import org.publicvalue.multiplatform.oidc.appsupport.WasmCodeAuthFlowFactorytoimport org.publicvalue.multiplatform.oidc.appsupport.WebMainCodeAuthFlowFactory