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

Vuex store getter for currently logged in User #3404

Merged
merged 12 commits into from
Apr 20, 2023

Conversation

DeNic0la
Copy link
Collaborator

@DeNic0la DeNic0la commented Apr 8, 2023

After #3403 i searched for a better way.
I made a getter to get the logged in User from the Store.

this is my attempt/version at fixing the auth.user situation @carlobeltrame
(#3403 (comment))

Fixes #2994

Copy link
Member

@carlobeltrame carlobeltrame left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! Let me show you how we simplified working with the API in eCamp v3.

frontend/src/plugins/store/auth.js Outdated Show resolved Hide resolved
@carlobeltrame
Copy link
Member

Code looks great now. I'll deploy so we can easily manually check all places that could be affected by this.

@carlobeltrame carlobeltrame added the deploy! Creates a feature branch deployment for this PR label Apr 9, 2023
@github-actions
Copy link

github-actions bot commented Apr 9, 2023

Feature branch deployment currently inactive.

If the PR is still open, you can add the deploy! label to this PR to trigger a feature branch deployment.

Copy link
Member

@carlobeltrame carlobeltrame left a comment

Choose a reason for hiding this comment

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

I found no bugs, only a few places which I would like to have adjusted as well.

In src/views/Camps.vue there is also a usage of auth.user which is only used on the mobile view, and which would be cleaner to replace with the new getter. I couldn't trigger a bug there either, but you never know.

frontend/src/App.vue Outdated Show resolved Hide resolved
@DeNic0la
Copy link
Collaborator Author

i removed profile prop, from App.vue and updated to getter on the mentioned places and some other places where I stumbled upon auth.user
it worked on my Machine

@DeNic0la DeNic0la mentioned this pull request Apr 18, 2023
@@ -117,16 +118,9 @@ export default {
}
},
},
mounted() {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it's a good idea to completely remove the reload on mounted. E.g. in case the profile was updated in another tab or on another device in the meantime. But should be good enough for now, since this should rarely be the case, and since we don't have live updates yet anyways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wouldn't it be better to do that in a Routeguard?
If the current solution is good enough for the MVP or for now could you merge it since I don't have the authorization to do so?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting idea with the route guards. We haven't done that in any other view as far as I'm aware. Some disadvantages with the route guards are that they are only available in view components (not in reusable sub-components such as the Picasso component) and that the reloading logic is further away from the code which actually uses the data, making it easier to become outdated when refactoring. Do you have any specific plus points in mind for the route guards?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@carlobeltrame
if the user object is loaded over the getter it will return the latest instance/version from the apiStore no matter where it was loaded.
A mutation of the apiStore.user as well as the change of the authenticated user id ( auth.user._meta.self ) will refresh the state everywhere (meaning in every component that uses the getter correct). So the guard is not available in a reusable sub-component but it doesn't matter because updating the user is like changing a variable in a Singleton.

There are UI Components that require the authenticated user (for example UserMeta.vue) and are used in multiple views. If a View requires the user itself it is perfectly fine to reload the user from the API in that View in a mounted() hook (like in the Case of Profile.vue).
But if a View doesn't require the user in the Component rendered by the router there are only terrible options imo:

  • You eighter reload the user in a mounted() hook of a Component that doesn't require the user itself (Not that pretty and risk someone removing it because they think you just forgot to remove it).
  • You reload the user in a UI component that gets used across multiple Views (Some of the other views might already reload the user themself)
  • You reload the user somewhere like the NavigationDefault.vue or something with no regard if it actually gets used in the View. This is not that terrible with something like the user since every View (excluding Login/Register) I have seen does require the user itself or in a sub-component but this doesn't sound like a best practice to me.

Things get really bad if you have a View that doesn't require the user but uses multiple sub-components that require the user.
In the worst case, you end up with the user being reloaded from the API multiple times per view (and perhaps with multiple network requests)

My approach for Vue would be to use the Meta Fields and declare that a rout should reload the user and implement the reload in a beforeAll/afterAll hook.
Or on the requireAuth() before the next() call if you are fine with possibly reloading the user on a few routes that don't actually require it. (There is a certain closeness to reloading the user object and requiring authentication )

However you are completely right, the logic for reloading the user is separated from the code using the data, and when removing the last component that requires the user from a View I highly doubt anyone would remember to remove the meta field. But the user is not something that belongs to a component or a view. It is a global state. it doesn't belong to a route eighter but a route is a better place to refresh the global state than a components mounted() hook imo.

I don't know how, if, or when you plan to implement live updates but until then the best solution imo is that if a user wants to be logged in 2 pages/devices and updates anything on the user object he eighter refreshes the page manually or waits for the JWT to reload (tho I am not 100% sure that works).

I usually work with Angular and my approach & opinions are probably very biased.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detailed analysis.
If we start using router meta for this, I'd like to do it consistently for all types of API data which all components use. The case with the logged in user is easy, but it gets more involved once a component needs to reload one row of a storyboard (3 column block programme) inside of one specific block in one specific period (subcamp) of a specific camp.
I lean towards your second solution, and it might not be as bad as you think. In the small library which we wrote for accessing the ecamp API data, when reloading something, we already detect any ongoing reload requests for the same URI, and short-circuit if one is found. So the main downside against having each component reload their own data (if needed) is already resolved, and therefore I still think that the mounted hook of each component is the correct place to reload data.

@carlobeltrame carlobeltrame merged commit 877d2bb into ecamp:devel Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy! Creates a feature branch deployment for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing profile names is not reactive
3 participants