Skip to content

feat: add guest user #795

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.

Sign up for GitHub

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

Merged
skjnldsv merged 2 commits into from
May 7, 2025
Merged

feat: add guest user #795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c574f9c
feat: add guest user
skjnldsv May 2, 2025
56c6fa1
chore: deps bumps
skjnldsv May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions lib/guest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,68 @@
* SPDX-License-Identifier: GPL-3.0-or-later
*/
import { getBuilder } from '@nextcloud/browser-storage'
import { NextcloudUser } from './user'
import { emit } from '@nextcloud/event-bus'

const browserStorage = getBuilder('public').persist().build()

class GuestUser implements NextcloudUser {

private _displayName: string | null
Copy link
Contributor

@susnux susnux May 7, 2025

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

@skjnldsv skjnldsv May 7, 2025

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

@susnux susnux May 7, 2025

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

@skjnldsv skjnldsv May 7, 2025

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

@susnux susnux May 7, 2025

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^^).

readonly uid: string
readonly isAdmin: boolean

constructor() {
if (!browserStorage.getItem('guestUid')) {
browserStorage.setItem('guestUid', self.crypto.randomUUID())
}

this._displayName = browserStorage.getItem('guestNickname') || ''
this.uid = browserStorage.getItem('guestUid') || self.crypto.randomUUID()
this.isAdmin = false

}

get displayName(): string | null {
return this._displayName
}

set displayName(displayName: string) {
this._displayName = displayName
browserStorage.setItem('guestNickname', displayName)
emit('user:info:changed', this)
}

}

let currentUser: GuestUser | undefined

/**
* Get the currently Guest user or null if not logged in
*/
export function getGuestUser(): GuestUser {
Copy link
Contributor

@susnux susnux May 7, 2025

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.

if (!currentUser) {
currentUser = new GuestUser()
}

return currentUser
}

/**
* Get the guest nickname for public pages
*/
export function getGuestNickname(): string | null {
return browserStorage.getItem('guestNickname')
return getGuestUser()?.displayName || null
}

/**
* Set the guest nickname for public pages
* @param nickname The nickname to set
*/
export function setGuestNickname(nickname: string): void {
browserStorage.setItem('guestNickname', nickname)
if (!nickname || nickname.trim().length === 0) {
throw new Error('Nickname cannot be empty')
}

getGuestUser().displayName = nickname
}
2 changes: 1 addition & 1 deletion lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ export type { CsrfTokenObserver } from './requesttoken'
export type { NextcloudUser } from './user'

export { getCSPNonce } from './csp-nonce'
export { getGuestNickname, setGuestNickname } from './guest'
export { getGuestUser, getGuestNickname, setGuestNickname } from './guest'
export { getRequestToken, onRequestTokenUpdate } from './requesttoken'
export { getCurrentUser } from './user'
Loading