-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix(space-nuxt-base): refactor useAppBridge() #68
Conversation
const { completed, appBridgeAuth, oauth } = useAppBridge(); | ||
|
||
const nuxtApp = useNuxtApp(); | ||
nuxtApp.provide('appBridge', { | ||
completed, | ||
appBridgeAuth, | ||
oauth, | ||
}); | ||
</script> | ||
|
||
<template> | ||
<slot v-if="!config.appBridge.enabled || status === 'authenticated'" /> | ||
<div v-else-if="status === 'authenticating'"></div> | ||
<div v-else-if="status === 'error'"></div> | ||
<slot v-if="!config.appBridge.enabled || completed" /> |
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've actually done slightly more than just refactoring.
I'm providing appBridge
here, so that users can access $appBridge
in their projects.
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.
So this basically create app bridge globally accessible in the frontend? @eunjae-lee
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.
yes!
@@ -1,3 +1,5 @@ | |||
import { APP_BRIDGE_TOKEN_HEADER_KEY } from '../../utils/const'; |
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 had to explicitly import this, because nuxt-starter
project failed to auto-import this. I guess the same auto import configuration should be at nuxt-starter to make it work without this import. But we can revisit that later.
event.context.appSession = appSession; | ||
|
||
const afterAuthenticated = appConfig.auth.middleware?.afterAuthenticated; | ||
if (typeof afterAuthenticated === 'function') { | ||
return await afterAuthenticated({ event, appSession }); | ||
} |
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've moved this part as a separate middleware. More explanation is in the file.
regarding Nuxt Layer
…e, so I spreaded it and now completed is just a boolean
c2f11f1
to
34e4b26
Compare
window.parent.postMessage(payload, getParentHost()); | ||
}; | ||
|
||
const useAppBridgeAuth = ({ |
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 be also extracted as type type UseAppBridge = (params: {authenticated: ()=> Promise<void>}) => void
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.
but this is a nit pick so no need to implement
What?
This PR refactors
useAppBridge()
for better maintainability.It's a bit hard to diff the code, because I moved things around.
One notable change is that I added a param
authenticated()
touseAppBridgeAuth({ authenticated })
, so that I can initiate oauth flow right after finishing app bridge auth flow.