Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: array subscriptions #657

Merged

Conversation

NtTestAlert
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #654

What is the new behavior?

  • Decorator metadata is not overwritten
  • Added new decorator @OnEvents

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@NtTestAlert NtTestAlert changed the title feat: array subscriptions #654 feat: array subscriptions Oct 17, 2022
*
* @param events array of events to subscribe to
*/
export const OnEvents = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of introducing a new decorator, could we just modify the @OnEvent implementation to use the "extendArrayMetadata" under the hood?

Copy link
Contributor Author

@NtTestAlert NtTestAlert Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original decorator already accepts string|string[], so introducing string[]|string[][] would not be backwards compatible (can't tell if the string array is an array of events, or a single event with array representing the namespaces)

The PR changes allow OnEvent to be used multiple times on the same method properly, just the new decorator was added to allow conflicting signatures without breaking backwards compat (OnEvent calls OnEvents internally with a single element array).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying that we should update the decorator signature, but just use the "extendArrayMetadata" function under the hood, as you did in the OnEvents decorator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without OnEvents decorator it would still be impossible to attach events based on a dynamic array (say, based on a config class etc)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, we don't want to support such a feature in this package

Copy link
Contributor Author

@NtTestAlert NtTestAlert Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I can remove this, but this would force users who need this use-case (incl me and commenters on the original issue[s]) to resort to monkey-patching decorator metadata with custom code instead of an easy to use decorator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to dynamically retrieve event names (and dynamically register handlers for them), you shouldn't use decorators in the first place

Copy link
Contributor Author

@NtTestAlert NtTestAlert Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example of dynamic parameters in decorators from nestjs docs https://docs.nestjs.com/fundamentals/custom-providers#class-providers-useclass
This will be impossible (say IS_DEV?['event1', 'event2']:['event1'])


Removing OnEvents from PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a different use case than what's stated in the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@NtTestAlert NtTestAlert force-pushed the feat/event-array-subscriptions branch 2 times, most recently from 677b251 to 052fef4 Compare October 31, 2022 02:19
@sorsaffari
Copy link

looking forward to this being published 🚀

@NtTestAlert
Copy link
Contributor Author

Rebased to newest master.
@kamilmysliwiec any update on this?

@Hatko
Copy link

Hatko commented Jan 12, 2023

A nice feature - we spent some time figuring out why passing an array doesn't work

@NtTestAlert
Copy link
Contributor Author

NtTestAlert commented Jan 15, 2023

I've made this fix in ~1h since first opening a github issue.
We are at 3 months, nearing 2 months of radio silence...

I hope everything is alright with @kamilmysliwiec - assuming so, since still active on gh.
Honestly I just want to see this fixed, so I can remove the workarounds and have cleaner code.

In the meantime, my fork can be used by adding

    "@nestjs/event-emitter": "github:NtTestAlert/event-emitter#1.3.1-multi",

to package.json should anyone want to use it as-is.
(added npm run build to prepare, so it can be installed directly as a github dependency)

the fork includes a "multi event" decorator, which is optional.
the other interface is and will remain fully compatible with vanilla.

Might publish npm package of the fork later, if longer term solution is required

@kamilmysliwiec
Copy link
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants