feat(js): js sdk feeds module #5688

LetItRock · 2024-06-06T15:29:57Z

What changed? Why was the change needed?

The changes include:

@novu/client ApiService types improvements
The build configuration for ESM, CJS - preserving folders structure, UMD - minification and gzip
Improvements to the size limits checker script
The Feeds module
The Notification ORM pattern
Event Emitter types improvements

Screenshots

The UMD size limits checker script

Testing the JS SDK

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

linear · 2024-06-06T15:29:59Z

COM-29 Implement Feeds

netlify · 2024-06-06T15:32:06Z

❌ Deploy Preview for novu-design failed. Why did it fail? →

Name	Link
🔨 Latest commit	`ab164a5`
🔍 Latest deploy log	https://app.netlify.com/sites/novu-design/deploys/66741e720f96850008fa2ab9

LetItRock · 2024-06-06T15:32:25Z

packages/client/src/api/api.service.ts

+  private removeNullUndefined(obj) {
+    return Object.fromEntries(
+      Object.entries(obj).filter(([_, value]) => value != null)
+    );
+  }


the helper function that is used on the URL query params to remove the keys with null or undefined values

nit-pick comment: Alternatively we can use the URLSearchParams interface.

it doesn't work :(

LetItRock · 2024-06-06T15:33:14Z

packages/client/src/api/api.service.ts

+    executedType: `${ButtonTypeEnum}`,
+    status: `${MessageActionStatusEnum}`,


this is the way to convert enum keys to union, this way we don't require using the enum from @novu/shared

LetItRock · 2024-06-06T15:33:25Z

libs/shared/src/entities/messages/action.enum.ts

@@ -1,5 +1,4 @@
 export enum ButtonTypeEnum {
  PRIMARY = 'primary',
  SECONDARY = 'secondary',
-  CLICKED = 'clicked',


LetItRock · 2024-06-06T15:34:01Z

packages/client/src/api/api.service.ts

+  /**
+   * @deprecated use markMessagesAs instead
+   */


the endpoint is deprecated so this function as well

LetItRock · 2024-06-06T15:34:33Z

packages/client/src/api/api.service.ts

+  async markMessagesAs({
+    messageId,
+    markAs,
+  }: {
+    messageId: string | string[];
+    markAs: `${MarkMessagesAsEnum}`;
+  }): Promise<INotificationDto[]> {
+    return await this.httpClient.post(`/widgets/messages/mark-as`, {
+      messageId,
+      markAs,
+    });
+  }


the new function that should be used instead

LetItRock · 2024-06-06T15:59:29Z

packages/js/src/feeds/notification.ts

    this.cta = notification.cta;
    this.payload = notification.payload;
    this.overrides = notification.overrides;
  }
+
+  markAsRead(): Promise<Notification> {


the interface for some of these functions is simpler and doesn't require passing any args

LetItRock · 2024-06-06T16:00:13Z

packages/js/src/feeds/types.ts

+import type { NotificationActionStatus, NotificationButton, NotificationStatus } from '../types';
+import { Notification } from './notification';
+
+export type FetchFeedArgs = {


types for all function args from feeds module

LetItRock · 2024-06-06T16:00:37Z

packages/js/src/novu.ts

+    ApiServiceSingleton.getInstance({ backendUrl: options.backendUrl });
+    this.#emitter = NovuEventEmitter.getInstance({ recreate: true });


the singleton are created with this class initialization

LetItRock · 2024-06-06T16:01:34Z

packages/js/test-sdk.ts

@@ -0,0 +1,43 @@
+import { Novu } from './src';


the test file to verify the sdk, I use it with command tsx ./test-sdk.ts

LetItRock · 2024-06-06T16:01:49Z

packages/js/tsconfig.json

+    "noImplicitAny": true,
+    "strictNullChecks": true,


stricter ts checks

SokratisVidros · 2024-06-07T05:24:44Z

packages/js/src/base-module.ts

+    this._emitter = NovuEventEmitter.getInstance();
+    this._apiService = ApiServiceSingleton.getInstance();


Alternatively we can use a factory pattern and get the event emitter from the closure. Testing would be slightly easier in that case due to dependency injection (DI).

SokratisVidros · 2024-06-07T05:27:52Z

packages/js/src/feeds/feeds.ts

+  }
+
+  async markNotificationActionAs(args: MarkNotificationActionAsByIdArgs): Promise<Notification>;
+  async markNotificationActionAs(args: MarkNotificationActionAsByInstanceArgs): Promise<Notification>;


Let’s add specific method per action for better DX.

Then we should do the same for the other functions as well, like the same way I did in the Notification class:

markNotificationAsRead, markNotificationAsSeen, markNotificationAsUnseen, markNotificationAsUnread

markNotificationPrimaryActionAsDone, markNotificationPrimaryActionAsPending, markNotificationSecondaryActionAsDone, markNotificationSecondaryActionAsPending

But then the question would be whether we should also emit separate events?

SokratisVidros · 2024-06-07T05:31:41Z

packages/js/src/types.ts

+  DONE = 'done',
+}
+
+export enum ActorType {


This enum is convoluted. Is its purpose to drive the icon selection?

the none status should be replaced with the null type. The as far as I can tell there is a user vs system actor.

Id also argue that it might be more helpful to replace it with an icon property.

I'm not sure either if it's useful to the users at all, like they are just interested in the actor icon URL and nothing more.
But that's just what we have rn and we can improve later.

https://linear.app/novu/issue/COM-34/improve-the-actor-interface

SokratisVidros · 2024-06-07T05:32:16Z

packages/js/src/types.ts

+
+export type Session = {
+  token: string;
+  profile: {


Suggested change

profile: {

subscriber:

I responded in another PR: https://github.com/novuhq/novu/pull/5665/files#r1630782252. I'll create a ticket for that.

https://linear.app/novu/issue/COM-33/refactor-the-session-interface

BiswaViraj

Looks good,
Have added couple of small comments

BiswaViraj · 2024-06-07T11:16:38Z

packages/js/src/feeds/helpers.ts

+    case NotificationStatus.SEEN:
+      return { seen: true };
+    case NotificationStatus.UNSEEN:
+      return { seen: false };


Suggested change

return { seen: false };

return { seen: false, read: false };

I didn't add that just because we haven't fixed that on the server side. Like to not confuse the users with an optimistic state different from the result (server) state.

BiswaViraj · 2024-06-07T11:17:23Z

packages/js/src/feeds/helpers.ts

+      return { read: notification.read, seen: notification.seen };
+    case NotificationStatus.SEEN:
+    case NotificationStatus.UNSEEN:
+      return { seen: notification.seen };


Suggested change

return { seen: notification.seen };

return { seen: notification.seen, read: notification.read };

BiswaViraj · 2024-06-07T11:21:21Z

packages/js/src/feeds/feeds.ts


 export class Feeds extends BaseModule {
-  async fetch({ page = 1, ...restOptions }: FetchFeedOptions): Promise<PaginatedResponse<Notification>> {
+  async fetch({ page = 0, status, ...restOptions }: FetchFeedArgs = {}): Promise<PaginatedResponse<Notification>> {


💭 thought (non-blocking): I think we should start page numbers from 1
and handle internally by page = --page;‏

yeah, everywhere in our APIs we are using a page from 0, and in sdks as well :(

SokratisVidros · 2024-06-12T14:32:46Z

packages/client/src/api/api.service.ts

+  private removeNullUndefined(obj) {
+    return Object.fromEntries(
+      Object.entries(obj).filter(([_, value]) => value != null)
+    );
+  }


nit-pick comment: Alternatively we can use the URLSearchParams interface.

SokratisVidros · 2024-06-12T14:34:25Z

packages/client/src/api/api.service.ts

+    messageId: string | string[];
+    markAs: `${MarkMessagesAsEnum}`;
+  }): Promise<INotificationDto[]> {
+    return await this.httpClient.post(`/widgets/messages/mark-as`, {


In the new API, I'd like to suggest having a dedicated method per action in a RESTful way. /v1/inbox/messages/:id/mark-as-read without any body. This is not actionable for now.

I'll create a ticket.

SokratisVidros · 2024-06-12T14:35:29Z

packages/js/src/base-module.ts

+    this._emitter = NovuEventEmitter.getInstance();
+    this._apiService = ApiServiceSingleton.getInstance();


Correct, that's where DI helps.

SokratisVidros · 2024-06-12T15:06:45Z

packages/js/src/event-emitter/types.ts

+export type Events = SessionInitializeEvents &
+  FeedFetchEvents &
+  FeedFetchCountEvents &
+  FeedMarkNotificationsAsEvents &
+  FeedMarkAllNotificationsAsEvents &
+  FeedRemoveNotificationsEvents &
+  FeedRemoveAllNotificationsEvents &
+  NotificationMarkAsEvents &
+  NotificationMarkActionAsEvents &
+  NotificationRemoveEvents;


I am torn about this one. It feels a bit convoluted if we start from the event naming convention, deconstruct the parts, and then define the payload based on that. How about the opposite: start with an event map and build types from it?

For example.

SokratisVidros · 2024-06-12T15:09:08Z

packages/js/src/feeds/feeds.ts

-        const response = await this._apiService.getNotificationsList(page, restOptions);
+        const response = await this._apiService.getNotificationsList(page, {
+          ...restOptions,
+          ...(status && SEEN_OR_UNSEEN.includes(status) && { seen: status === NotificationStatus.SEEN }),


This conversion of status between the SDK and the apiService seems unnecessary. Is it mandatory to keep the apiService backward compatible?

Unfortunately, it is a public package :( but I think that we would need to deprecate it and suggest using the new SDK instead

SokratisVidros · 2024-06-12T15:11:38Z

packages/js/src/feeds/feeds.ts

+    );
+  }
+
+  async markNotificationsAs(args: MarkNotificationsAsByIdsArgs): Promise<Notification[]>;


I suggest we go with a specific method on this public API for better DX, such as novu.feeds.markAllAsRead().

If you are saying to go additionally with these then yes we can introduce this granularity, but need to be consistent with that pattern and implement also for other methods:

for single notification: markNotificationAsRead, markNotificationAsUnread, markNotificationAsSeen, markNotificationAsUnseen

for selected notifications: markNotificationsAsRead, markNotificationsAsUnread, markNotificationsAsSeen, markNotificationsAsUnseen

for all notifications: markAllAsRead, markAllAsUnread, markAllAsSeen, markAllAsUnseen

similar pattern to actions

And then the question is whether all these should be treated as a separate events

* feat(js): js sdk feeds module (#5688) * feat(js): improve the package json exports and tsup config * feat(js): lazy session initialization and interface fixes * feat(js): js sdk feeds module * feat(js): js sdk preferences (#5701) * feat(js): js sdk preferences * feat(js): handling the web socket connection and events (#5704) * feat(js): handling the web socket connection and events * feat: ui solid --------- Co-authored-by: Biswajeet Das <[email protected]> * fix: caching for session initialize * fix: worker --------- Co-authored-by: Paweł Tymczuk <[email protected]> Co-authored-by: Biswajeet Das <[email protected]> Co-authored-by: Dima Grossman <[email protected]>

LetItRock requested review from SokratisVidros and BiswaViraj June 6, 2024 15:29

LetItRock self-assigned this Jun 6, 2024

LetItRock temporarily deployed to Linting June 6, 2024 15:30 — with GitHub Actions Inactive

LetItRock had a problem deploying to Linting June 6, 2024 15:30 — with GitHub Actions Failure

github-actions bot added @novu/notification-center @novu/shared labels Jun 6, 2024

LetItRock commented Jun 6, 2024

View reviewed changes

SokratisVidros requested changes Jun 7, 2024

View reviewed changes

LetItRock mentioned this pull request Jun 7, 2024

feat(js): lazy session initialization and interface fixes #5665

Merged

LetItRock force-pushed the com-29-js-sdk-feeds branch from 0a7eea7 to 8e8c303 Compare June 7, 2024 08:19

LetItRock had a problem deploying to Linting June 7, 2024 08:19 — with GitHub Actions Failure

LetItRock temporarily deployed to Linting June 7, 2024 08:19 — with GitHub Actions Inactive

LetItRock temporarily deployed to Linting June 7, 2024 08:24 — with GitHub Actions Inactive

LetItRock had a problem deploying to Linting June 7, 2024 08:24 — with GitHub Actions Failure

LetItRock temporarily deployed to Linting June 7, 2024 08:24 — with GitHub Actions Inactive

LetItRock requested a review from SokratisVidros June 7, 2024 08:48

LetItRock had a problem deploying to Linting June 7, 2024 09:35 — with GitHub Actions Failure

LetItRock temporarily deployed to Linting June 7, 2024 09:35 — with GitHub Actions Inactive

BiswaViraj reviewed Jun 7, 2024

View reviewed changes

LetItRock requested a review from BiswaViraj June 7, 2024 14:55

Base automatically changed from com-28-js-sdk-session-lazy-initialization to next June 11, 2024 08:20

LetItRock force-pushed the com-29-js-sdk-feeds branch from 9448d67 to e62dd85 Compare June 11, 2024 08:28

LetItRock temporarily deployed to Linting June 11, 2024 08:28 — with GitHub Actions Inactive

LetItRock had a problem deploying to Linting June 11, 2024 08:28 — with GitHub Actions Failure

LetItRock temporarily deployed to Linting June 11, 2024 10:27 — with GitHub Actions Inactive

LetItRock temporarily deployed to Linting June 11, 2024 11:25 — with GitHub Actions Inactive

LetItRock temporarily deployed to Linting June 11, 2024 15:41 — with GitHub Actions Inactive

LetItRock temporarily deployed to Linting June 12, 2024 10:48 — with GitHub Actions Inactive

SokratisVidros requested changes Jun 12, 2024

View reviewed changes

LetItRock force-pushed the com-29-js-sdk-feeds branch from 69aa508 to 4fd00de Compare June 13, 2024 09:53

LetItRock temporarily deployed to Linting June 13, 2024 09:53 — with GitHub Actions Inactive

LetItRock added 8 commits June 20, 2024 14:14

feat(js): the base js sdk package scaffolding

34fd4ef

feat(js): improve the package json exports and tsup config

aabe028

feat(js): lazy session initialization and interface fixes

a7787e7

feat(js): renamed the *.spec.ts to *.test.ts

2971ee6

feat(js): js sdk feeds module

ac22931

feat(js): updated the tsup config file

6160bb9

feat(js): improved notification class fields

953d335

chore(js): spell check fix

ab164a5

LetItRock force-pushed the com-29-js-sdk-feeds branch from 4fd00de to ab164a5 Compare June 20, 2024 12:19

LetItRock temporarily deployed to Linting June 20, 2024 12:20 — with GitHub Actions Inactive

LetItRock merged commit 9088aab into next Jun 20, 2024
28 checks passed

LetItRock deleted the com-29-js-sdk-feeds branch June 20, 2024 13:47

		executedType: `${ButtonTypeEnum}`,
		status: `${MessageActionStatusEnum}`,

		ApiServiceSingleton.getInstance({ backendUrl: options.backendUrl });
		this.#emitter = NovuEventEmitter.getInstance({ recreate: true });

		this._emitter = NovuEventEmitter.getInstance();
		this._apiService = ApiServiceSingleton.getInstance();

	return { seen: notification.seen };
	return { seen: notification.seen, read: notification.read };

feat(js): js sdk feeds module #5688

feat(js): js sdk feeds module #5688

Conversation

LetItRock commented Jun 6, 2024 • edited Loading

What changed? Why was the change needed?

Screenshots

The UMD size limits checker script

Testing the JS SDK

Related enterprise PR

Special notes for your reviewer

linear bot commented Jun 6, 2024

netlify bot commented Jun 6, 2024 • edited Loading

❌ Deploy Preview for novu-design failed. Why did it fail? →

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

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

BiswaViraj 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

Choose a reason for hiding this comment

LetItRock commented Jun 6, 2024 •

edited

Loading

netlify bot commented Jun 6, 2024 •

edited

Loading