Set TypeScript to strict mode and fix issues related to server types #261

jbaez · 2022-06-24T22:46:28Z

Added new npm commands to run lint and check code. We would normally just use npm run check:all which does all the checks.

Fixed all the lint and TypeScript issues, removing unused imports and variables, adding types and interfaces, and updating the code where needed.

The commits in this PR have more info on the changes made

- set TypeScript in strict mode - add npm commands to lint / check code - fix all lint issues - fix some TS issues - rename User reponse DTO to make it consistent with the other ones - override Express/User interface to use UserResponseDto interface This is for when the accessing the `user` from a Express Request, like in `asset-upload-config`

- fix all the remaining TypeScript errors - add missing `@types/mapbox__mapbox-sdk` package

alextran1502

Thank you for another high-quality PR to improve the code base! I've left some comments and an idea to fix some issues while running on the dev server. Let me know what do you think

alextran1502 · 2022-06-25T01:37:03Z

server/libs/database/src/entities/user.entity.ts

@@ -3,32 +3,32 @@ import { Column, CreateDateColumn, Entity, PrimaryGeneratedColumn } from 'typeor
 @Entity('users')
 export class UserEntity {
  @PrimaryGeneratedColumn('uuid')
-  id: string;
+  id!: string;


I assume ! further indicating that this property must exist

Yes exactly, it's called definite assignment assertion. In general we should only be using these in Entities where properties are not null, because TypeORM cannot use a constructor to initialise the values. So we know that after the Entity is "hydrated" by TypeORM, those properties would have its value.

alextran1502 · 2022-06-25T01:38:54Z

server/apps/immich/src/api-v1/album/album-repository.ts

@@ -39,7 +39,7 @@ export class AlbumRepository implements IAlbumRepository {
  ) {}

  async create(ownerId: string, createAlbumDto: CreateAlbumDto): Promise<AlbumEntity> {
-    return await getConnection().transaction(async (transactionalEntityManager) => {


Why does strict mode suggest getting rid of await?

It doesn't, I just removed it because in these case is not useful. When using await what it does is, it resolves the promise, so we now have the "album entity", then it wraps it in another resolved promise which is then returned.

Without the await it just returns the promise. A good practice is to have the return values defined in the function (don't let TypeScript infer), in this case Promise<AlbumEntity>, so whatever you do inside you must still return that type or TypeScript will complain.

alextran1502 · 2022-06-25T01:50:40Z

server/apps/immich/src/api-v1/album/album.service.spec.ts

@@ -25,6 +25,7 @@ describe('Album service', () => {
    albumEntity.createdAt = 'date';
    albumEntity.sharedUsers = [];
    albumEntity.assets = [];
+    albumEntity.albumThumbnailAssetId = null;


Hmm I wonder prior to this change, I was still able to get the photo thumbnail

The entity has defined albumThumbnailAssetId as nullable: true. So when TypeORM "hydrated" the entity, it would set it to null or string. This spec was not setting albumThumbnailAssetId to anything (undefined), so the type was not matching what TypeORM would "hydrate" it to, and TypeScript was complaining because I used undefined (instead of null) later in one of the tests for checking the expectation.

alextran1502 · 2022-06-25T02:24:11Z

server/apps/immich/src/api-v1/asset/asset.service.ts

@@ -166,6 +194,10 @@ export class AssetService {
        res.set({
          'Content-Type': 'image/jpeg',
        });
+        if (!asset.resizePath) {
+          Logger.error('Error serving IMAGE asset for web ');


I think this should be Logger.error('Error serving IMAGE asset for web', 'ServeFile'); and the line below will be throw new InternalServerErrorException(Failed to serve image asset for web);

I based those lines in the ones below in line 198-199:

Logger.error('Error serving IMAGE asset ', e); throw new InternalServerErrorException(`Failed to serve image asset ${e}`, 'ServeFile');

Looks like all InternalServerErrorException in this file use ServeFile as the second param. So I think it is consistent with the other ones.

Agree, we could also add ServeFile as the second argument of Logger.error, since we don't have an error to pass as in there (like in the other places).

Let me know what you think

alextran1502 · 2022-06-25T02:28:20Z

server/apps/immich/src/api-v1/asset/asset.service.ts

@@ -44,10 +55,14 @@ export class AssetService {
    asset.modifiedAt = assetInfo.modifiedAt;
    asset.isFavorite = assetInfo.isFavorite;
    asset.mimeType = mimeType;
-    asset.duration = assetInfo.duration;
+    asset.duration = assetInfo.duration || null;


So from the mobile app, if the asset is an image, it still return the duration and it will be "0:00:00.00000" so I think this line will be

asset.duration = assetInfo.duration || '0:00:00.00000';

So we need to fix the duration type as well

OK, then the best place to fix it would be in the AssetResponseDto. So this line would correct, because that's just how it's saved in the DB. And I'll update the AssetResponseDto so duration is of type string, and when we map the entity to the DTO, if duration is null, it would return 0:00:00.00000

alextran1502 · 2022-06-25T02:41:40Z

server/apps/immich/src/config/asset-upload.config.ts

-      const yearInfo = new Date(fileInfo.createdAt).getFullYear();
-      const monthInfo = new Date(fileInfo.createdAt).getMonth();
+      // const yearInfo = new Date(fileInfo.createdAt).getFullYear();
+      // const monthInfo = new Date(fileInfo.createdAt).getMonth();


I would it let sit here as a reminder :P

alextran1502 · 2022-06-25T02:46:57Z

server/apps/immich/src/modules/background-task/background-task.processor.ts

-          console.log('error deleting ', asset.originalPath);
-        }
-      });
+      // TODO: what if there is no asset.resizePath. Should fail the Job?


If there is no asset.resizePath then it should try and then log out like below.

I think your current implementation work!

Before I put the if, when asset.resizePath was undefined it would probably throw an error. Since I made it safe adding the if, it would not longer throw an error.

I imagine the Job might fail if there is an unhandled error thrown 🤔. An error from the callback of fs.unlink doesn't seem like it would make the Job fail either. Maybe it should use something like job.moveToFailed(), or maybe just ignoring it it's fine in this case 😄

alextran1502 · 2022-06-25T02:51:45Z

server/apps/immich/src/app.controller.ts

@@ -2,5 +2,10 @@ import { Controller, Get, Res, Headers } from '@nestjs/common';
 import { Response } from 'express';
 @Controller()
 export class AppController {
-  constructor() {}
+  @Get()
+  async redirectToWebpage(@Res({ passthrough: true }) res: Response, @Headers() headers: Record<string, string>) {


This has been removed on the main branch.

Oops, sorry my bad. I had a merge conflict which I solved the wrong way.

alextran1502 · 2022-06-25T02:53:24Z

server/apps/microservices/src/processors/metadata-extraction.processor.ts

@@ -128,7 +127,7 @@ export class MetadataExtractionProcessor {
        });
      }
    } catch (error) {
-      Logger.error(`Failed to trigger object detection pipe line ${error.toString()}`);
+      Logger.error(`Failed to trigger object detection pipe line ${String(error)}`);


Didn't know there is such method :O wow

🙂 it's handy in this case, where error could be anything, not just an Error instance with a toString method.

alextran1502 · 2022-06-25T03:04:29Z

server/apps/immich/src/config/asset-upload.config.ts


      if (file.fieldname == 'assetData') {
-        const originalUploadFolder = `${basePath}/${req.user['id']}/original/${req.body['deviceId']}`;
+        const originalUploadFolder = `${basePath}/${req.user.id}/original/${req.body['deviceId']}`;


So I got this error when trying to run the app

And the type of user here is User from Express.User

The user here is returned from the function validate in jwt.strategy.ts which is a UserEntity

Here is my proposed fix for this issue in asset-upload.config.ts and profile-image-upload.config.ts

Yes sorry about that, the e2e test was working in my computer (this is also the reason why e2e tests failed in CI) because the container was already created and still had the old tsconfig inside, so no strict mode 😅.

I have added a much neater fix, but would probably need to move the global.d.ts somewhere inside src, I'll take a look at fixing this.
Using as should be the last resort in TypeScript (also any but this is a subject for another day 😄)

I had added a short explanation in the git commit message:

override Express/User interface to use UserResponseDto interface This is for when the accessing the `user` from a Express Request, like in `asset-upload-config`

So Express left the User interface empty, so you can override it with your own interface like

declare global { namespace Express { // eslint-disable-next-line @typescript-eslint/no-empty-interface interface User extends UserResponseDto {} } }

I've used UserResponseDto for now, although we can create a specific interface for the user returned in jwt.strategy

This is now of type `string` that defaults to '0:00:00.00000' if not set which is what the mobile app currently expects

Use `ServeFile` as the context for logging an error when asset.resizePath is not set

`redirectToWebpage` was removed in main as is no longer used.

jbaez · 2022-06-25T11:34:12Z

Thank you for another high-quality PR to improve the code base! I've left some comments and an idea to fix some issues while running on the dev server. Let me know what do you think

My pleasure. I've fixed the issues and replied the comments. Let me know your thoughts.

jbaez added 3 commits June 24, 2022 23:55

Fix the rest of TS issues

2b1a288

- fix all the remaining TypeScript errors - add missing `@types/mapbox__mapbox-sdk` package

Merge remote-tracking branch 'origin/main' into fix/server-types

5d4f570

jbaez requested a review from alextran1502 June 24, 2022 22:46

alextran1502 requested changes Jun 25, 2022

View reviewed changes

jbaez added 4 commits June 25, 2022 13:14

Move global.d.ts to server src folder

fb1e056

Update AssetReponseDto duration type

faa7bc2

This is now of type `string` that defaults to '0:00:00.00000' if not set which is what the mobile app currently expects

Set context when logging error in asset.service

79841c6

Use `ServeFile` as the context for logging an error when asset.resizePath is not set

Fix wrong AppController merge conflict resolution

f324ed2

`redirectToWebpage` was removed in main as is no longer used.

jbaez requested a review from alextran1502 June 25, 2022 11:34

matthinc mentioned this pull request Jun 25, 2022

Fix error with geocodingClient being null #263

Closed

alextran1502 approved these changes Jun 25, 2022

View reviewed changes

alextran1502 merged commit c918f5b into main Jun 25, 2022

alextran1502 deleted the fix/server-types branch June 25, 2022 19:07

zcyang9111 mentioned this pull request Dec 15, 2023

[Snyk] Security upgrade docusaurus-preset-openapi from 0.6.4 to 0.7.0 zcyang9111/immich#7

Closed

zcyang9111 mentioned this pull request Jan 6, 2024

[Snyk] Security upgrade docusaurus-preset-openapi from 0.6.4 to 0.7.0 zcyang9111/immich#16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set TypeScript to strict mode and fix issues related to server types #261

Set TypeScript to strict mode and fix issues related to server types #261

jbaez commented Jun 24, 2022

alextran1502 left a comment

alextran1502 Jun 25, 2022

jbaez Jun 25, 2022

alextran1502 Jun 25, 2022

jbaez Jun 25, 2022

alextran1502 Jun 25, 2022

jbaez Jun 25, 2022

alextran1502 Jun 25, 2022

jbaez Jun 25, 2022

alextran1502 Jun 25, 2022

jbaez Jun 25, 2022

alextran1502 Jun 25, 2022

alextran1502 Jun 25, 2022

alextran1502 Jun 25, 2022

jbaez Jun 25, 2022

alextran1502 Jun 25, 2022 •

edited

Loading

jbaez Jun 25, 2022 •

edited

Loading

alextran1502 Jun 25, 2022

jbaez Jun 25, 2022

alextran1502 Jun 25, 2022

alextran1502 Jun 25, 2022 •

edited

Loading

alextran1502 Jun 25, 2022

jbaez Jun 25, 2022

jbaez commented Jun 25, 2022

Set TypeScript to strict mode and fix issues related to server types #261

Set TypeScript to strict mode and fix issues related to server types #261

Conversation

jbaez commented Jun 24, 2022

alextran1502 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextran1502 Jun 25, 2022 • edited Loading

Choose a reason for hiding this comment

jbaez Jun 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextran1502 Jun 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaez commented Jun 25, 2022

alextran1502 Jun 25, 2022 •

edited

Loading

jbaez Jun 25, 2022 •

edited

Loading

alextran1502 Jun 25, 2022 •

edited

Loading