Skip to content

Conversation

@skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented May 2, 2025

Features:

  • Creates a new getGuestUser method that looks similar to a normal user
  • Add a way to monitor displayName change to so whatever you want (reload an avatar, or change other stuff...)

Code snippet example

// Watching for displayname change in vue component
data() {
	return {
		displayName: getGuestUser().displayName,
	}
},

mounted() {
	subscribe('user:info:changed', (guest) => { this.displayName = guest.displayName })
},

@skjnldsv skjnldsv requested a review from susnux May 2, 2025 15:15
@skjnldsv skjnldsv self-assigned this May 2, 2025
@skjnldsv skjnldsv added enhancement New feature or request 3. to review labels May 2, 2025
@skjnldsv skjnldsv force-pushed the feat/guest-user branch from 07e3865 to 6254690 Compare May 2, 2025 15:17
@susnux
Copy link
Contributor

susnux commented May 2, 2025

Still not fully sure about the event thing, would it make more sense as an event-bus event ?

I think the events make sense when you listen to a specific object / instance.
In this case it is both (as the guest user is unique) meaning its both:

  • a global state
  • an instance event

Maybe it makes things easier to work with (with current design of nextcloud-auth) when having a global event like auth:guest:display-name-changed (or any other wording 😅 ).
But I am fine with both, whatever you find easier to work with.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented May 6, 2025

Maybe it makes things easier to work with (with current design of nextcloud-auth) when having a global event like auth:guest:display-name-changed (or any other wording 😅 ).

user:properties:changed ?
So we can also use it for the NextcloudUser ?

That would differenciate for other like

  • user:preferences:changed
  • user:config:changed
  • user:info:changed
    or something ?

@skjnldsv skjnldsv force-pushed the feat/guest-user branch 2 times, most recently from 114b450 to b9568cc Compare May 7, 2025 06:52
Signed-off-by: skjnldsv <[email protected]>
@skjnldsv skjnldsv force-pushed the feat/guest-user branch from b9568cc to c574f9c Compare May 7, 2025 06:52
@skjnldsv
Copy link
Contributor Author

skjnldsv commented May 7, 2025

Done! With tests!
Using user:info:changed as I think it would make sense later to extend the Nextcloud user too and have more infos like email, pronouns... etc

Signed-off-by: skjnldsv <[email protected]>
/**
* Get the currently Guest user or null if not logged in
*/
export function getGuestUser(): GuestUser {
Copy link
Contributor

Choose a reason for hiding this comment

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

Either

Suggested change
export function getGuestUser(): GuestUser {
export function getGuestUser(): NextcloudUser {

Or we would need to also export the type of GuestUser.


class GuestUser implements NextcloudUser {

private _displayName: string | null
Copy link
Contributor

Choose a reason for hiding this comment

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

If we would export this class type (skip this depending on the other comment), then the type includes this private field.
Also it is accessible and only marked private for typescript.

You might consider instead using standard ES visibility:

Suggested change
private _displayName: string | null
#displayName: string | null

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 thought # was dropped in the end? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

No they are standardized in es2019:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_properties

Its more like protected but enforced by runtime, meaning you cannot access the property from outside the class.

Whereas the typescript private, protected, and readonly are just types which get stripped away:

Screenshot 2025-05-07 at 16-35-39 TypeScript TS Playground - An online editor for exploring TypeScript and JavaScript

Meaning you could still access those private fields, while #something will throw an exception:
grafik


In the end its not really important, both have good reasons to use or not to use, it depends :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, absolutely!
But I remember using the # a while back, when it was drafted and about to be accepted.
But I also remember that we rolled back and stopped using it 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

there are some places using it, but it really is not that important.
If you mess with private fields you better know what you are doing, so its your risk.

Meaning its just a matter of taste if you prefer Typescript sugar or Javascript syntax.
E.g. a benefit of Typescript sugar is that you can unit test it (while also there are some guidelines that discurage you from testing private properties, but thats another topic^^).

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

@skjnldsv skjnldsv merged commit 42ebe14 into main May 7, 2025
11 checks passed
@skjnldsv skjnldsv deleted the feat/guest-user branch May 7, 2025 09:01
@susnux
Copy link
Contributor

susnux commented May 7, 2025

Damn did not saw the auto merge 😮‍💨

@skjnldsv
Copy link
Contributor Author

skjnldsv commented May 7, 2025

Damn did not saw the auto merge 😮‍💨

#799

@skjnldsv skjnldsv mentioned this pull request May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants