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

Add reactivity to oauthClient #18

Open
wants to merge 1 commit into
base: vue-2
Choose a base branch
from
Open

Add reactivity to oauthClient #18

wants to merge 1 commit into from

Conversation

jjnesbitt
Copy link
Member

Previous to this change, the "logout" button would not change to "login" if clicked (it wasn't reactive). Logout would still be performed, but the text wouldn't change, leading to a situation where clicking "logout" actually logs you in.

It's a small change but IMO the main benefit is in displaying how to correctly use the oauth client. It's also possible this is the wrong approach, in which case I'm open to suggestions.

@jjnesbitt jjnesbitt requested a review from subdavis November 3, 2021 18:35
Copy link
Contributor

@subdavis subdavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right way to add reactivity to this state.

  • The OauthClient library itself should probably be a vue instance that chooses specific properties (such as user state) to be reactive rather than making an unfamiliar class instance reactive(), which might have weird side effects. vue-project-template shouldn't be responsible for peering into the black box of OauthClient.
  • If this isn't possible, then reactivity should be layered in before provide: { oauthClient } in main.ts in this template, because otehrwise you're making a shallow copy of the client in Home.vue and using oauthClient anywhere else in the app won't do anything.

@subdavis
Copy link
Contributor

FWIW: do not add a vue dependency to OauthClient: it will make a 2-to-3 transition much harder. Let's resolve this in here.

@brianhelba
Copy link
Member

I think it'd be out of scope to add anything Vue-specific to the OAuthClient anyway. The OAuthClient is intended to be framework-agnostic. If there are standardized things that'd make reactivity easier downstream, that might be more acceptable.

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.

3 participants