Conversation
sergeyteleshev
force-pushed
the
5764-cb-bump-npm-dependencies
branch
from
d88a5a5 to
2be2da5
Compare
SychevAndrey
left a comment
SychevAndrey
left a comment
There was a problem hiding this comment.
Some changes look unrelated to lib updates and/or change app's logic
| } | ||
|
|
||
| return this.administrationItemService.canDeActivate(screen, toScreen, this.isConfigurationMode, screen.item !== toScreen?.item); | ||
| return await this.administrationItemService.canDeActivate(screen, toScreen, this.isConfigurationMode, screen.item !== toScreen?.item); |
There was a problem hiding this comment.
why do we need return await here?
There was a problem hiding this comment.
Cause linter says async function should have await
If I remove async, TypeScript says: "You return Promise", but the function has no async
So this is the compromise where ESLint and TypeScript are fine with the code
There was a problem hiding this comment.
As I can see that function returns boolen | Promise<boolean>, not only Promise<boolean>
It's better to fix return type
| switch (value) { | ||
| case 'DEFAULT': | ||
| return 'DEFAULT'; | ||
| case 'COMBINE': |
There was a problem hiding this comment.
i'm not sure why we adding this in update lib ticket
There was a problem hiding this comment.
I agree. Why did we make these changes?
There was a problem hiding this comment.
I changed the order of the functions. It should remain the same behavior
| availableDrivers: schema.array(schema.string()), | ||
| requiredNetworkHandlersIds: schema.array(schema.string()), | ||
| connectionId: schema.string().or(schema.undefined()), | ||
| connectionId: schema.string().or(schema.null()), |
There was a problem hiding this comment.
what was the cause of this changes?
There was a problem hiding this comment.
Previously, this code led to this interface:
connectionId: schema.string().or(schema.undefined()) -> {connectionId: string | undefined }
Now it leads to this interface:
connectionId: schema.string().or(schema.undefined()) -> {connectionId?: string }
We need this interface not to be optional according to the code
There was a problem hiding this comment.
connectionId: schema.string().or(schema.null()) -> {connectionId: string | null }
There was a problem hiding this comment.
here i've reverted to .or(schema.undefined()) and removed .required()
| "@testing-library/jest-dom": "^6", | ||
| "@testing-library/react": "^16", | ||
| "@types/node": "^22", | ||
| "@types/node": "^24", |
There was a problem hiding this comment.
Please keep the ^22 version for node; it's our current minimal supported node version
| stringedBoolean() { | ||
| return z.union([z.enum(['false', '0']).transform(() => false), z.boolean(), z.string(), z.number()]).pipe(z.coerce.boolean()); | ||
| }, | ||
| locales: new Map<string, any>([]), |
There was a problem hiding this comment.
locales should be private, you can move it to module scope:
const locales = new Map<string, any>([]);
export const schemaExtra = {| switch (value) { | ||
| case 'DEFAULT': | ||
| return 'DEFAULT'; | ||
| case 'COMBINE': |
There was a problem hiding this comment.
I agree. Why did we make these changes?
| availableDrivers: schema.array(schema.string()), | ||
| requiredNetworkHandlersIds: schema.array(schema.string()), | ||
| connectionId: schema.string().or(schema.undefined()), | ||
| connectionId: schema.string().or(schema.null()), |
| availableDrivers: schema.array(schema.string()), | ||
| requiredNetworkHandlersIds: schema.array(schema.string()), | ||
| connectionId: schema.string().or(schema.undefined()), | ||
| connectionId: schema.string().or(schema.null()), |
There was a problem hiding this comment.
connectionId: schema.string().or(schema.null()) -> {connectionId: string | null }
| availableDrivers: schema.array(schema.string()), | ||
| requiredNetworkHandlersIds: schema.array(schema.string()), | ||
| connectionId: schema.string().or(schema.undefined()), | ||
| connectionId: schema.string().or(schema.null()), |
There was a problem hiding this comment.
here i've reverted to .or(schema.undefined()) and removed .required()
4 participants
Removed zod translation packages, cause zod 4 supports it now - https://zod.dev/error-customization#internationalization.
Also, they added
z.stringbool(), so I replaced our solution with theirs - https://zod.dev/v4?id=stringboolcloses https://github.com/dbeaver/pro/issues/5764