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.

Sign up for GitHub

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

Jump to bottom

fix(router): support non-NgFactory promise in loadChildren typings #29832

Closed
wants to merge 1 commit into from
Closed

fix(router): support non-NgFactory promise in loadChildren typings #29832

wants to merge 1 commit into from

Conversation

filipesilva
Copy link
Contributor

@filipesilva filipesilva commented Apr 11, 2019 •
edited

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
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@filipesilva filipesilva requested review from a team as code owners April 11, 2019 13:24
filipesilva
filipesilva commented Apr 11, 2019
View reviewed changes
packages/router/src/config.ts Outdated
export type LoadChildrenCallback = () => Type<any>| NgModuleFactory<any>|
Promise<NgModuleFactory<any>>| Promise<Type<any>>| Observable<Type<any>>;
export type LoadChildrenCallback = () => Type<any>| NgModuleFactory<any>| Observable<Type<any>>|
Promise<NgModuleFactory<any>| Type<any>| any>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alxhub is there a something better than Promise<any> for JIT NgModules here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Examples of errors due to the missing typings can be seen on the failing CLI tests: https://circleci.com/gh/angular/angular-cli/51697 and https://circleci.com/gh/angular/angular-cli/51700.

@filipesilva filipesilva requested a review from alxhub April 11, 2019 13:25
@mary-poppins
Copy link

You can preview c96947b at https://pr29832-c96947b.ngbuilds.io/.

@filipesilva filipesilva force-pushed the loadchildren-nonfactory-promise branch from c96947b to a498f7e Compare April 11, 2019 13:37
@mary-poppins
Copy link

You can preview a498f7e at https://pr29832-a498f7e.ngbuilds.io/.

filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Apr 13, 2019
@filipesilva
test: workaround Router typings
28dd070 
This workaround should be removed after angular/angular#29832 is released.
@filipesilva filipesilva added target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 13, 2019
@filipesilva filipesilva mentioned this pull request Apr 13, 2019
Merged
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Apr 13, 2019
@filipesilva
test: workaround Router typings
293d578 
This workaround should be removed after angular/angular#29832 is released.
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Apr 13, 2019
@filipesilva
test: workaround Router typings
0338c8d 
This workaround should be removed after angular/angular#29832 is released.
filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Apr 13, 2019
@filipesilva
test: workaround Router typings
d2225c5 
This workaround should be removed after angular/angular#29832 is released.
alexeagle pushed a commit to angular/angular-cli that referenced this pull request Apr 13, 2019
@filipesilva @alexeagle
test: workaround Router typings
e2a3895 
This workaround should be removed after angular/angular#29832 is released.
alxhub
alxhub approved these changes Apr 16, 2019
View reviewed changes
packages/router/src/config.ts
@@ -97,8 +97,8 @@ export type ResolveData = {
* @see `Route#loadChildren`.
* @publicApi
*/
export type LoadChildrenCallback = () => Type<any>| NgModuleFactory<any>|
Promise<NgModuleFactory<any>>| Promise<Type<any>>| Observable<Type<any>>;
export type LoadChildrenCallback = () => Type<any>| NgModuleFactory<any>| Observable<Type<any>>|
Copy link
Member

Choose a reason for hiding this comment

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

The |any in Promise<NgModuleFactory | Type | any> makes the other two redundant. They're useful for documentation purposes, I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's why I didn't want to remove them. If you think it should be different, I'll change.

@filipesilva filipesilva removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 16, 2019
@alxhub alxhub added the action: merge The PR is ready for merge by the caretaker label Apr 16, 2019
@alxhub alxhub closed this in 2bfb6a0 Apr 16, 2019
@filipesilva filipesilva deleted the loadchildren-nonfactory-promise branch April 16, 2019 18:07
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@filipesilva @wKoza
fix(router): support non-NgFactory promise in loadChildren typings (a…
43795ed 
…ngular#29832)

PR Close angular#29832
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kara kara kara approved these changes

@alxhub alxhub alxhub approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@filipesilva @mary-poppins @kara @alxhub @googlebot