server: misc services code cleanup #275

norm · 2022-12-07T20:01:11Z

norm commented

2022-12-07 20:01:11 +00:00

No description provided.

norm force-pushed refactor/services from 2d97af5acc to 5a442bb7bb

2022-12-07 20:04:26 +00:00

Compare

norm changed title from ~~server: misc code cleanup~~ to server: misc services code cleanup

2022-12-07 20:05:04 +00:00

Johann150 reviewed 2022-12-07 22:02:06 +00:00

packages/backend/src/services/user-cache.ts Outdated

					
				@ -6,3 +7,3 @@

				export const userByIdCache = new Cache<CacheableUser>(

					Infinity,

					(id) => Users.findOneBy({ id }).then(x => x ?? undefined),

					async (id) => id ? (await Users.findOneBy({ id }) ?? undefined) : undefined,

Johann150 commented

2022-12-07 22:01:59 +00:00

I don't think checking whether id is undefined is necessary. Similar for the other caches here. It would not make sense for the cache key to be undefined.

I don't think checking whether `id` is undefined is necessary. Similar for the other caches here. It would not make sense for the cache key to be undefined.

norm marked this conversation as resolved

Johann150 commented

2022-12-07 22:16:50 +00:00

The cache in services/relay.ts uses null as the only cache key. I guess that one could use an empty string instead? (Also not sure if that's the only place that had a cache with only one key, please check.)

The cache in `services/relay.ts` uses `null` as the only cache key. I guess that one could use an empty string instead? (Also not sure if that's the only place that had a cache with only one key, please check.)

norm commented

2022-12-07 23:02:04 +00:00

Kinda confused with the localUserByNativeTokenCache. It uses the user's token as a key. But according to the comment for the token field:

The native access token of the User. It will be null if the origin of the user is local.

Not sure what's the best approach for this.

Kinda confused with the `localUserByNativeTokenCache`. It uses the user's `token` as a key. But according to the comment for the `token` field: > The native access token of the User. It will be null if the origin of the user is local. Not sure what's the best approach for this.

Johann150 commented

2022-12-08 17:02:17 +00:00

I'm pretty sure that comment is wrong, because that would make no sense. The native token is used by the "native" frontend. It would make no sense for remote users to have a token.

The comment should probably be:

The native access token of local users, or null.

I'm pretty sure that comment is wrong, because that would make no sense. The native token is used by the "native" frontend. It would make no sense for remote users to have a token. The comment should probably be: > The native access token of local users, or null.

Johann150 reviewed 2022-12-08 18:57:11 +00:00

packages/backend/src/misc/cache.ts Outdated

					
				@ -3,2 +2,3 @@

					public cache: Map<string, { date: number; value: T; }>;

					private lifetime: number;

					public fetcher: (key: string | null) => Promise<T | undefined>;

					public fetcher: (key: string) => Promise<T | undefined>;

Johann150 commented

2022-12-08 18:45:50 +00:00

Does the type definition also have to say async?

Does the type definition also have to say `async`?

norm commented

2022-12-08 20:00:01 +00:00

I don't think it's necessary since it's a function that returns a promise. async is syntactic sugar that wraps the function's return value in a promise.

I don't think it's necessary since it's a function that returns a promise. `async` is syntactic sugar that wraps the function's return value in a promise.

Johann150 marked this conversation as resolved

packages/backend/src/services/user-cache.ts Outdated

					
				@ -35,3 +36,3 @@

								}

								if (Users.isLocalUser(user)) {

									localUserByNativeTokenCache.set(user.token, user);

									localUserByNativeTokenCache.set(user.token ?? '', user);