Feature/request scoped event emitters #544

thiagomini · 2022-07-07T20:31:44Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Request Scoped listeners are not available right now.

What is the new behavior?

We can inject the EVENT_PAYLOAD symbol and make our listeners Request Scoped:

class EventListener {
  constructor(@Inject(EVENT_PAYLOAD) private readonly eventPayload: any) { }
}

Does this PR introduce a breaking change?

Yes
No

Other information

You can follow the commits to understand how it became from the first version to the refactored one, which I used to reduce the complexity of the EventSubscribersLoader class.

thiagomini · 2022-07-07T20:32:53Z

tests/src/events-provider.request-scoped.consumer.ts

+    }
+  }
+
+  public static injectedEventPayload = {};


The reason why I created this static property is to make it easy to check the event payload was injected in the tests. Otherwise we would need a specific contextId

thiagomini · 2022-07-07T20:33:21Z

tsconfig.json

@@ -9,7 +9,7 @@
    "esModuleInterop": true,
    "experimentalDecorators": true,
    "target": "es6",
-    "sourceMap": false,
+    "sourceMap": true,


Changed this to enable local debug. It was really helpful in my experience here

delucca

Great job 🚀

I'm not going to approve it, since I don't know if this could break something. But I added a few minor comments 😄

delucca · 2022-07-07T20:53:05Z

lib/event-subscribers.loader.ts

+              instance,
+              methodKey,
+              isRequestScoped,
+              wrapper.host as Module,


There is no other way of doing this without using as? Just to avoid losing type-safety

I tried a couple of things but apparently this is the less hacky way of doing it. To give you more context on that, the reason Why I had to do it is because of the following:

The InstanceWrapper.host is an optional attribute, as you can see here

This host attribute is used by the injector.loadPerContext method in this same file

loadPerContext method requires a module attribute, hence it's not optional

this project has strict flag set to true, as you can see in the tsconfig.json

The reason 4 is the actual culprit of this as Module statement. You can see in the nestjs/bull package the module is handed over the function with no casting, and that's because its tsconfig file does not have strict: true

if it's optional, can't you just if it?

You mean to add an if statement before calling subscribeToEventIfListener ?

Yes.
Maybe like

if(!wrapper.host) { throw new Error() }

Hmmmm I see, but do you think this is really the expected behavior here? To be honest, I'm not sure what would happen if this host really didn't exist when we tried to inject the instance. @kamilmysliwiec do you have some opinion on that as wel?

At that time, host will never be undefined so the as Module assertion is correct here.

lib/event-subscribers.loader.ts

delucca · 2022-07-07T20:58:29Z

lib/event-subscribers.loader.ts

+    */
+
+    const payloadObjectOrArray =
+      eventPayload.length > 1 ? eventPayload : eventPayload[0];


I'm not 100% sure about the context here, but if eventPayload can be a string this conditional would not work, since 'foo'.length is 1.

In other words. To be sure that something is an array you need to use the static Array.isArray()

I'll double check this, but I think the eventPayload here will actually always be an array, even if we passed a string. That's because the eventPayload here is actually an alias for the function's args parameters, which is a rest parameter (and thus, an array)

I added a test for the case you mentioned in this commit and it works as expected, therefore I think we don't have to check against an array here

lib/event-subscribers.loader.ts

kamilmysliwiec · 2022-07-08T13:28:06Z

Fantastic PR @thiagomini, thank you 🙌

…in an event listener

…anual declaration

refactor request-scoped event payloads to increase the readability and reduce the burden to add other request payload types in the future

thiagomini · 2022-07-08T16:42:31Z

Fantastic PR @thiagomini, thank you 🙌

Thank you, Kamil! I added a few other commits here to resolve the review suggestions. I also refactored a bit the test code to use constants instead of manually set objects. In the final commit, I refactored a bit of the EventsProviderRequestScopedConsumer to look cleaner. Before this commit, I was adding more and more static variables to the class to handle different event payloads, so I decided to move this logic away to another object, the RequestScopedEventPayload class.

lib/event-subscribers.loader.ts

kamilmysliwiec · 2022-07-11T07:50:30Z

Left 1 minor comment, otherwise, it should be good to go!

rename EventSubscribersLoader private method

thiagomini added 4 commits July 7, 2022 16:47

feat(): add first version of request scoped listeners

26426c2

refactor(): refactor EventSubscribersLoader methods

a2bbb69

test(): remove unused import

0934666

refactor(): rename event payload symbol

883d13f

thiagomini commented Jul 7, 2022

View reviewed changes

delucca reviewed Jul 7, 2022

View reviewed changes

kamilmysliwiec reviewed Jul 8, 2022

View reviewed changes

lib/event-subscribers.loader.ts Outdated Show resolved Hide resolved

thiagomini added 4 commits July 8, 2022 11:34

refactor(): apply PR suggestions

64555d0

test(e2e): add test to verify a string payload is correctly injected …

afeeb0c

…in an event listener

test(e2e): refactor tests to use constant event payloads instead of m…

929a3ea

…anual declaration

test(): refactor request-scoped event payload

b434bd6

refactor request-scoped event payloads to increase the readability and reduce the burden to add other request payload types in the future

thiagomini requested a review from kamilmysliwiec July 8, 2022 16:42

kamilmysliwiec reviewed Jul 11, 2022

View reviewed changes

lib/event-subscribers.loader.ts Outdated Show resolved Hide resolved

refactor(): rename method

c382818

rename EventSubscribersLoader private method

thiagomini requested a review from kamilmysliwiec July 11, 2022 14:03

kamilmysliwiec merged commit c209f33 into nestjs:master Jul 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/request scoped event emitters #544

Feature/request scoped event emitters #544

thiagomini commented Jul 7, 2022

thiagomini Jul 7, 2022

thiagomini Jul 7, 2022

delucca left a comment

delucca Jul 7, 2022

thiagomini Jul 7, 2022 •

edited

Sikora00 Jul 8, 2022

thiagomini Jul 8, 2022

Sikora00 Jul 11, 2022

thiagomini Jul 11, 2022

kamilmysliwiec Jul 12, 2022

delucca Jul 7, 2022

thiagomini Jul 7, 2022

thiagomini Jul 8, 2022

kamilmysliwiec commented Jul 8, 2022

thiagomini commented Jul 8, 2022

kamilmysliwiec commented Jul 11, 2022

Feature/request scoped event emitters #544

Feature/request scoped event emitters #544

Conversation

thiagomini commented Jul 7, 2022

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delucca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagomini Jul 7, 2022 • edited

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

kamilmysliwiec commented Jul 8, 2022

thiagomini commented Jul 8, 2022

kamilmysliwiec commented Jul 11, 2022

thiagomini Jul 7, 2022 •

edited