feat(core): more precise type for APP_INITIALIZER token

[abarghoud]

This commit updates the type of the APP_INITIALIZER injection token to
better document the expected types of values that Angular handles. Only
Promises and Observables are awaited and other types of values are ignored,
so the type of APP_INITIALIZER has been updated to
Promise<unknown> | Observable<unknown> | void to reflect this behavior.

BREAKING CHANGE:

The type of the APP_INITIALIZER token has been changed to more accurately
reflect the types of return values that are handled by Angular. Previously,
each initializer callback was typed to return any, this is now
Promise<unknown> | Observable<unknown> | void. In the unlikely event that
your application uses the Injector.get or TestBed.inject API to inject
the APP_INITIALIZER token, you may need to update the code to account for
the stricter type.

Additionally, TypeScript may report the TS2742 error if the APP_INITIALIZER
token is used in an expression of which its inferred type has to be emitted
into a .d.ts file. To workaround this, an explicit type annotation is needed,
which would typically be Provider or Provider[].

Closes #40729

Does this PR introduce a breaking change?

• Yes
• No

[abarghoud]

@JoostK do you have any idea of why tests are failing with the following error:

ERROR: /home/circleci/ng/packages/router/BUILD.bazel:5:10: Compiling Angular templates (ViewEngine - devmode) //packages/router:router failed: (Exit 1): ngc-wrapped.sh failed: error executing command 
  (cd /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular && \
  exec env - \
  bazel-out/host/bin/packages/bazel/src/ngc-wrapped/ngc-wrapped.sh '--node_options=--expose-gc' '--node_options=--stack-trace-limit=100' '--node_options=--max-old-space-size=4096' @@bazel-out/k8-fastbuild/bin/packages/router/router_es5_tsconfig.json)
Execution platform: //tools:rbe_ubuntu1604-angular
packages/router/src/router_module.ts:587:17 - error TS2742: The inferred type of 'provideRouterInitializer' cannot be named without a reference to '../../../external/npm/node_modules/rxjs'. This is likely not portable. A type annotation is necessary.

587 export function provideRouterInitializer() {

[JoostK]

@JoostK do you have any idea of why tests are failing with the following error:

ERROR: /home/circleci/ng/packages/router/BUILD.bazel:5:10: Compiling Angular templates (ViewEngine - devmode) //packages/router:router failed: (Exit 1): ngc-wrapped.sh failed: error executing command 
  (cd /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular && \
  exec env - \
  bazel-out/host/bin/packages/bazel/src/ngc-wrapped/ngc-wrapped.sh '--node_options=--expose-gc' '--node_options=--stack-trace-limit=100' '--node_options=--max-old-space-size=4096' @@bazel-out/k8-fastbuild/bin/packages/router/router_es5_tsconfig.json)
Execution platform: //tools:rbe_ubuntu1604-angular
packages/router/src/router_module.ts:587:17 - error TS2742: The inferred type of 'provideRouterInitializer' cannot be named without a reference to '../../../external/npm/node_modules/rxjs'. This is likely not portable. A type annotation is necessary.

587 export function provideRouterInitializer() {

Yeah, maybe this is why @AndrewKushnir marked this as breaking?

The issue here is that TypeScript uses its inferred type from the returned array literal as the return type of the function declaration that is emitted into the generated .d.ts file. Since it includes APP_INITIALIZER, its type is present somewhere in the inferred return type and this would therefore have to emit Observable and create a valid import for it. TS is telling you that the import path it determined for rxjs is unlikely to portable, which is true as that relative import path may not be valid in a different context (e.g. after the file has been published to NPM).

angular/packages/router/src/router_module.ts

Lines 587 to 598 in 4a7a649

	export function provideRouterInitializer() {
	return [
	RouterInitializer,
	{
	provide: APP_INITIALIZER,
	multi: true,
	useFactory: getAppInitializer,
	deps: [RouterInitializer]
	},
	{provide: ROUTER_INITIALIZER, useFactory: getBootstrapListener, deps: [RouterInitializer]},
	{provide: APP_BOOTSTRAP_LISTENER, multi: true, useExisting: ROUTER_INITIALIZER},
	];

The workaround is quite simple: add an explicit return type Provider[] to that function.

It's an interesting case considering the fact that this was not an issue with the named type; I'd be interested to hear what others think here w.r.t. having a named type as you initially had.

[AndrewKushnir]

Yeah, maybe this is why @AndrewKushnir marked this as breaking?

I was under impression that this would be breaking and was going to perform some research on which scenarios can be affected, but tests identified them faster :)

It's an interesting case considering the fact that this was not an issue with the named type; I'd be interested to hear what others think here w.r.t. having a named type as you initially had.

I like the current implementation (without exposing additional type), I think it's a more "lightweigth" API. I've started a set of tests in Google's codebase (internal-only link) to see how that would affect apps (+ also started a global run). We can use that data as an input to the discussion and final decision.

Thank you.

[AndrewKushnir]

Quick update after running tests in Google's codebase (all affected targets aka global run): there are no breakages caused by this change, which suggests that this changes is not that break-y (but still this is a breaking change as we've seen it with a Router example).

[AndrewKushnir]

@abarghoud I've left a couple suggestions to use ReadonlyArray instead of an Array, but overall the change looks good and it's not causing any issues in Google's codebase.

We'd also need to update commit message to include the "BREAKING CHANGE" note. We'll followup with a proposed draft to explain in which circumstances this might be an actual breaking change.

Thank you.

[JoostK]

@abarghoud Here's the suggestion for the commit message:

feat(core): more precise type for `APP_INITIALIZER` token

This commit updates the type of the `APP_INITIALIZER` injection token to
better document the expected types of values that Angular handles. Only
Promises and Observables are awaited and other types of values are ignored,
so the type of `APP_INITIALIZER` has been updated to
`Promise<unknown> | Observable<unknown> | void` to reflect this behavior.

BREAKING CHANGE:

The type of the `APP_INITIALIZER` token has been changed to more accurately
reflect the types of return values that are handled by Angular. Previously,
each initializer callback was typed to return `any`, this is now
`Promise<unknown> | Observable<unknown> | void`. In the unlikely event that
your application uses the `Injector.get` or `TestBed.inject` API to inject
the `APP_INITIALIZER` token, you may need to update the code to account for
the stricter type.

Additionally, TypeScript may report the TS2742 error if the `APP_INITIALIZER`
token is used in an expression of which its inferred type has to be emitted
into a .d.ts file. To workaround this, an explicit type annotation is needed,
which would typically be `Provider` or `Provider[]`.

Closes #40729

[abarghoud]

@JoostK @AndrewKushnir, I took into account your remarks as well as modified the commit message

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}