fix(infra): remove node package strange dependencies and move to shared #3588
Conversation
| @@ -1,7 +1,6 @@ | |||
| import { ArrayMaxSize, ArrayNotEmpty, IsArray, IsDefined, IsObject, IsOptional, IsString } from 'class-validator'; | |||
| import { ApiExtraModels, ApiProperty, ApiPropertyOptional, getSchemaPath } from '@nestjs/swagger'; | |||
| import { TriggerRecipientSubscriber, TriggerRecipients } from '@novu/node'; | |||
There was a problem hiding this comment.
We had a lot of dependencies of types from the Node package so I moved them to the Shared package as it would be expected.
| @@ -32,7 +32,6 @@ | |||
| "@nestjs/terminus": "9.2.1", | |||
| "@novu/application-generic": "^0.16.0", | |||
| "@novu/dal": "^0.16.0", | |||
| "@novu/node": "^0.16.0", | |||
There was a problem hiding this comment.
Worker app shouldn't have this dependency at all (plus not being used).
| export enum TriggerRecipientsTypeEnum { | ||
| SUBSCRIBER = 'Subscriber', | ||
| TOPIC = 'Topic', | ||
| } |
There was a problem hiding this comment.
Wrongly located as this is not a DTO.
| export interface ISubscriberPayload { | ||
| firstName?: string; | ||
| lastName?: string; | ||
| email?: string; | ||
| phone?: string; | ||
| avatar?: string; | ||
| locale?: string; | ||
| data?: SubscriberCustomData; | ||
| [key: string]: string | string[] | boolean | number | SubscriberCustomData | undefined; | ||
| } | ||
|
|
||
| export interface ISubscribersDefine extends ISubscriberPayload { | ||
| subscriberId: string; | ||
| } |
There was a problem hiding this comment.
More interfaces than DTOs.
| @@ -29,5 +30,3 @@ export interface IChannelCredentials { | |||
| webhookUrl?: string; | |||
| deviceTokens?: string[]; | |||
| } | |||
|
|
|||
| export type SubscriberCustomData = { [key: string]: string | string[] | boolean | number | undefined }; | |||
There was a problem hiding this comment.
More a type interface than a entity.
| @@ -23,7 +23,6 @@ | |||
| "dependencies": { | |||
| "@faker-js/faker": "^6.0.0", | |||
| "@novu/dal": "^0.16.0", | |||
| "@novu/node": "^0.16.0", | |||
There was a problem hiding this comment.
Testing lib shouldn't have this dependency at all.
| @@ -137,10 +141,6 @@ export class UserSession { | |||
| this.subscriberToken = token; | |||
| this.subscriberProfile = profile; | |||
| } | |||
|
|
|||
| this.serverSdk = new Novu(this.apiKey, { | |||
There was a problem hiding this comment.
Not used at all.
| @@ -68,7 +68,6 @@ | |||
| "@novu/mattermost": "^0.16.0", | |||
| "@novu/ms-teams": "^0.16.0", | |||
| "@novu/netcore": "^0.16.0", | |||
| "@novu/node": "^0.16.0", | |||
There was a problem hiding this comment.
Application generic shouldn't have this dependency at all (plus not being used).
p-fernandez
force-pushed
the
fix-stranger-dependencies-node-package
branch
from
86ca159
to
2de443f
Compare
| TopicKey, | ||
| TopicName, | ||
| TriggerRecipientsTypeEnum, | ||
| } from '@novu/shared'; | ||
|
|
||
| export interface ITopic { |
There was a problem hiding this comment.
Should live in Shared as it is reused.
|
|
||
| export interface INovuConfiguration { | ||
| backendUrl?: string; | ||
| } | ||
|
|
||
| export interface IAttachmentOptions { |
There was a problem hiding this comment.
Should live in Shared as it is reused.
| import { DigestUnitEnum, ISubscribersDefine } from '@novu/shared'; | ||
|
|
||
| import { IAttachmentOptions } from '../novu.interface'; | ||
| import { ITopic } from '../topics/topic.interface'; | ||
|
|
||
| export type TriggerRecipientSubscriber = string | ISubscribersDefine; | ||
| export type TriggerRecipientTopics = ITopic[]; | ||
|
|
||
| export type TriggerRecipient = TriggerRecipientSubscriber | ITopic; | ||
|
|
||
| export type TriggerRecipients = TriggerRecipient[]; | ||
|
|
||
| // string | ISubscribersDefine | (string | ISubscribersDefine | ITopic)[] | ||
| export type TriggerRecipientsPayload = | ||
| | TriggerRecipientSubscriber | ||
| | TriggerRecipients; |
There was a problem hiding this comment.
Should live in Shared as it is reused.
p-fernandez
force-pushed
the
fix-stranger-dependencies-node-package
branch
2 times, most recently
from
19af621
to
6395b8c
Compare
p-fernandez
force-pushed
the
fix-stranger-dependencies-node-package
branch
from
6395b8c
to
46d504f
Compare
| import { | ||
| IEmailOptions, |
There was a problem hiding this comment.
Struggling to understand the dependence of Application Generic with Stateless. 🤷🏻
There was a problem hiding this comment.
Could you please elaborate a bit what you mean?
There was a problem hiding this comment.
@ainouzgali Application generic are a set of use cases and services that are used mostly by API and Worker apps. As far as I understand, Stateless is a simplified API layer to be able to execute the providers added to Novu. So I can't figure out why either API or Worker should have a dependency on Stateless package.
There was a problem hiding this comment.
Only for the types it seems. I believe you can just create those types in Shared as well. As stateless is deprecated as far as I know. There should not be future changes in stateless.
There was a problem hiding this comment.
Yes, I moved some of the things I needed for now. But we might need to move or duplicate some more.
What change does this PR introduce?
Removes strange Node package dependencies from most of the apps and libs that are dependencies for either our infra or setup.
Why was this change needed?
I notice about this while trying to solve the failing tests of another PR. Part is inherited of a bad implementation of the Topics feature and other were inherit from previous refactors.
Note that we are missing to totally remove a dependency of the Node package from API app as all the invite system is depending on it (not ideal).
Other information (Screenshots)