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

fix(space-nuxt-base): refactor useAppBridge() #68

Merged
merged 10 commits into from
Jul 22, 2024

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Jul 18, 2024

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() to useAppBridgeAuth({ authenticated }), so that I can initiate oauth flow right after finishing app bridge auth flow.

@eunjae-lee eunjae-lee marked this pull request as ready for review July 18, 2024 10:00
@eunjae-lee eunjae-lee requested a review from BibiSebi July 18, 2024 10:00
Comment on lines +3 to +14
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" />
Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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';
Copy link
Contributor Author

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.

Comment on lines -31 to -36
event.context.appSession = appSession;

const afterAuthenticated = appConfig.auth.middleware?.afterAuthenticated;
if (typeof afterAuthenticated === 'function') {
return await afterAuthenticated({ event, appSession });
}
Copy link
Contributor Author

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.

Base automatically changed from feat/app-bridge to main July 22, 2024 14:02
@eunjae-lee eunjae-lee enabled auto-merge (squash) July 22, 2024 14:22
@eunjae-lee eunjae-lee disabled auto-merge July 22, 2024 14:23
@eunjae-lee eunjae-lee merged commit 778d6d9 into main Jul 22, 2024
1 check passed
@eunjae-lee eunjae-lee deleted the fix/refactor-app-bridge branch July 22, 2024 14:40
@eunjae-lee eunjae-lee restored the fix/refactor-app-bridge branch July 22, 2024 14:40
@eunjae-lee eunjae-lee deleted the fix/refactor-app-bridge branch July 22, 2024 14:40
window.parent.postMessage(payload, getParentHost());
};

const useAppBridgeAuth = ({
Copy link
Contributor

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

Copy link
Contributor

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

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.

2 participants