Conversation
|
Deploying preview environment to https://pr-26597.preview.internal.immich.build/ |
There was a problem hiding this comment.
in Zod semantics, .default(x) means “if the input is undefined, substitute x”, so the schema must accept undefined, which effectively makes that value optional-at-input. this causes the generated clients to mark these response properties as optional, but they always do have a value. Thus enforcing the type in ts is fine. All changes in the web commit relate to that.
mobile/lib/infrastructure/repositories/sync_stream.repository.dart
Outdated
Show resolved
Hide resolved
| if (result.success) { | ||
| added.add(result.id); | ||
| } else if (result.error == BulkIdResponseDtoErrorEnum.duplicate) { | ||
| } else if (result.error == BulkIdErrorReason.duplicate) { |
There was a problem hiding this comment.
before the enum was inlined in the album respons schema, now it has a namem/dedicated enum schema referenced
There was a problem hiding this comment.
additional tests for the feature introduced in #26532 and tests the zod implementation for it
| hideAt: Date | null; | ||
| type: MemoryType; | ||
| data: object; | ||
| data: Record<string, unknown>; |
There was a problem hiding this comment.
no runtime effect, but necessary for types to work out
timonrieger
force-pushed
the
refactor/zod-migration
branch
from
5f3615e to
d2529af
Compare
timonrieger
force-pushed
the
refactor/zod-migration
branch
3 times, most recently
from
6dd4a89 to
68d08fb
Compare
| longitude: number | null; | ||
| visibility: AssetVisibility; | ||
| stack: null; | ||
| stack: undefined; |
There was a problem hiding this comment.
this is the only type change I had to do, as there is a bug in nestja-zod that fails to output nullable schemas. This is non blocking as the server validation is still in place, just compile time type checks are failing. PR has been submitted: BenLorantfy/nestjs-zod#344
same applies to the web fix
| value: Map<String, Object>.from( | ||
| RemoteAssetMobileAppMetadata( | ||
| cloudId: mapping.localAsset.cloudId, | ||
| createdAt: mapping.localAsset.createdAt.toIso8601String(), | ||
| adjustmentTime: mapping.localAsset.adjustmentTime?.toIso8601String(), | ||
| latitude: mapping.localAsset.latitude?.toString(), | ||
| longitude: mapping.localAsset.longitude?.toString(), | ||
| ).toJson(), |
There was a problem hiding this comment.
ensures metadata is sent as a proper json body and satisfies the underlying type. The keys and values sent remain unchanged, so this has no breaking effect.
timonrieger
requested review from
danieldietzler and
shenlong-tanwen
as code owners
timonrieger
force-pushed
the
refactor/zod-migration
branch
3 times, most recently
from
48f6dd9 to
7aad864
Compare
timonrieger
force-pushed
the
refactor/zod-migration
branch
from
7aad864 to
1df08ce
Compare
timonrieger
force-pushed
the
refactor/zod-migration
branch
2 times, most recently
from
60dfcc0 to
803fbfe
Compare
timonrieger
force-pushed
the
refactor/zod-migration
branch
from
803fbfe to
bec91e9
Compare
timonrieger
force-pushed
the
refactor/zod-migration
branch
from
bec91e9 to
caef2b7
Compare
2 participants
see #26172 for the implementation proposal and #26427 for the first partial migration try.
As discussed, the migration effort is signifcantly lower and less akward when done in one go. This is the PR that does this!
I have iterated over this PR over and over again locally to get this as best as possible. This PR has been separated into 6 commits, one doing the actual server-side migration, one for tiny web, mobile and cli type fixes (unavoidable as explained in my review), the regeneration of the openapi spec/dart and ts clients and the removal of the
class-validatordependency. This approach should help making this reviewable.I tested the whole stack locally, server, web and mobile, going through plenty of views myself to check that it all works and no bugs are introduced. More testing from others is appreciated tho!