feat: add generic parameter for IEventPublisher in EventBus

[lucas-gregoire]

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?

• 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?

Currently, the EventBus.publish and EventBus.publishAll methods always return any, whereas they could infer types from the publish and publishAll methods of the IEventPublisher interface.

What is the new behavior?

This PR introduces a new generic parameter for the EventBus which represents the type of the event publisher (extending IEventPublisher), allowing to infer the return types of the publish and publishAll methods.

If the event publisher does not have a publishAll method—like in the case of DefaultPubSub—then the return type of publishAll is an array of the publish results from the publisher. This clarifies the behavior of the publishAll method.

By default, publish still returns any while publishAll now returns any[] which better matches the returned array.

Does this PR introduce a breaking change?

• Yes : publishAll now returns any[] instead of any by default. I think this is a "soft" breaking change.
• No

For this breaking change, here are the situations to consider:

• Users calling publishAll without using its result: no change.
• Users using the result of publishAll:
  • With the default event publisher or a custom publisher without a publishAll method:
    • Casting it to an array of something: this cast will still compile.
    • Casting it to something other than an array: this cast will no longer compile. While this situation doesn't make much sense, it is still possible. Users will have to adjust their code.
  • With a custom event publisher that implements a publishAll method:
    • Casting it to the result of publishAll: this cast will still compile but could be replaced by the inference allowed by the new generic parameter.

Personally, I think this is a breaking change with not much impact.

Other information

I also added some missing generics tests on query handlers.

[kamilmysliwiec]

@copilot resolve the merge conflicts in this pull request

[mrazauskas]

To draw your attention, it seems like vitest ignores this import. Try including a failing assertion (like expectTypeOf(false).toBeNumber();) in this file. Run the test from command line and it will pass.

The feature is marked experimental. It is not working as expected rather often. Here similar findings: ngrx/platform#5148 (comment) or flint-fyi/flint#2398 (comment).

[lucas-gregoire]

Thanks @mrazauskas, you were right, and it was worse: the assertions were never
checked at all. expectTypeOf(false).toBeNumber() passed with 44/44 green.

Digging in, both causes turn out to be documented behaviour rather than bugs:

• typecheck.enabled
  defaults to false, and nothing else in the pipeline ran tsc over test/,
  since npm run build only covers src.
• typecheck.include
  defaults to '**/*.{test,spec}-d.?(c|m)[jt]s?(x)', so a *.spec.ts file is
  simply never a type test. The run still printed Type Errors no errors.
• typecheck.tsconfig
  "tries to find closest tsconfig.json", which here is the root one, whose
  node10 resolution can't read Vitest 4's ESM-only type exports. With
  ignoreSourceErrors: false
  the run then exits 1 on 5 source errors while also printing
  Type Errors no errors.

So in this case it was our configuration rather than the feature misbehaving,
though I agree the reporting makes it very easy to miss, which is your point.

Fixed along the documented path: renamed to generics.spec-d.ts so it matches
the default include, set typecheck: { enabled: true, tsconfig: 'test/tsconfig.json' }, and moduleResolution: "bundler" in the test tsconfig.
And since the guide states that "Vitest
doesn't run these files, they are only statically analyzed by the compiler", I
dropped the try/catch + expect(true).toBeTruthy() scaffolding and the runtime
bus instances with it.

Verified by mutation rather than by a green run: forcing EventBus#publish back
to any in src/ now fails with 2 type errors, as does a wrong expectTypeOf
or a removed @ts-expect-error.

[kamilmysliwiec]

some new merge conflicts popped up after merging your other PR

[lucas-gregoire]

@kamilmysliwiec conflicts resolved, rebased on master

I also fixed @mrazauskas' review point along the way: the e2e type assertions
were never verified. I confirmed by mutation that a return-type regression
on EventBus#publish fails the run. build, test, test:e2e all pass
locally and on CI.

[mrazauskas]

So in this case it was our configuration rather than the feature misbehaving,
though I agree the reporting makes it very easy to miss, which is your point.

Yes, all is just about configuration.

Although the tool is reporting tests as passing, it does nothing in fact. For me, this looks rather dangerous. Any accidental change in the future will not be caught. As another example, nestjs/graphql#3936 is similarly adding type tests that do not run, but the report is all green again.

[lucas-gregoire]

Concretely, then. I regressed CommandBus#execute in src/, dropping its return
type from Promise<R> to Promise<any>:

npm run test:e2e fails on it in two places, exit 1:

With that same regression in place but typecheck removed from the config, the run reports 23 passed and exits 0. That is the "does nothing" state you're pointing at, and it is what this branch did before. Type regressions on those return types are caught now.