feat(core): add ModuleWithProviders generic type migration #33217

CaerusKaru · 2019-10-17T06:45:07Z

Static methods that return a type of ModuleWithProviders currently do not have to specify a type because the generic falls back to any. This is problematic because the type of the actual module being returned is not present in the type information.

Since Ivy uses d.ts files exclusively for downstream packages (rather than metadata.json files, for example), we no longer have the type of the actual module being created.

For this reason, a generic type should be added for ModuleWithProviders that specifies the module type. This will be required for all users in v10, but will only be necessary for users of Ivy in v9.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
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
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?

ModuleWithProviders requires an explicit generic type

Does this PR introduce a breaking change?

Yes
No

Other information

CaerusKaru · 2019-10-17T17:11:14Z

cc @devversion @IgorMinar @kara @vikerman

kara

Thanks for doing this! Initial thoughts below

tools/public_api_guard/core/core.d.ts

packages/core/src/metadata/ng_module.ts

packages/core/schematics/migrations/module-with-providers/README.md

packages/core/schematics/test/module_with_providers_migration_spec.ts

googlebot · 2019-10-17T20:41:48Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-10-17T20:46:35Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

packages/core/schematics/migrations/module-with-providers/README.md

packages/core/schematics/migrations/module-with-providers/module_collector.ts

packages/core/schematics/migrations/module-with-providers/transform.ts

packages/core/schematics/migrations/module-with-providers/util.ts

packages/core/schematics/migrations/module-with-providers/BUILD.bazel

packages/core/schematics/migrations/module-with-providers/index.ts

Static methods that return a type of ModuleWithProviders currently do not have to specify a type because the generic falls back to any. This is problematic because the type of the actual module being returned is not present in the type information. Since Ivy uses d.ts files exclusively for downstream packages (rather than metadata.json files, for example), we no longer have the type of the actual module being created. For this reason, a generic type should be added for ModuleWithProviders that specifies the module type. This will be required for all users in v10, but will only be necessary for users of Ivy in v9.

devversion

Nice work 💯 Just a few minor comments.

Also, the test cases look all very reasonable to me, so I think the migration covers most of the cases.

packages/core/schematics/migrations/module-with-providers/index.ts

packages/core/schematics/migrations/module-with-providers/transform.ts

packages/core/schematics/migrations/module-with-providers/util.ts

packages/core/schematics/test/module_with_providers_migration_spec.ts

kara

LGTM aside from a few minor nits below

packages/core/schematics/migrations/module-with-providers/README.md

packages/core/schematics/migrations/module-with-providers/transform.ts

devversion

LGTM

kara

LGTM

kara · 2019-10-21T17:21:56Z

VE presubmit

kara · 2019-10-21T18:41:17Z

Caretaker note: needs g3 patch (CL here)

…3217) Static methods that return a type of ModuleWithProviders currently do not have to specify a type because the generic falls back to any. This is problematic because the type of the actual module being returned is not present in the type information. Since Ivy uses d.ts files exclusively for downstream packages (rather than metadata.json files, for example), we no longer have the type of the actual module being created. For this reason, a generic type should be added for ModuleWithProviders that specifies the module type. This will be required for all users in v10, but will only be necessary for users of Ivy in v9. PR Close angular#33217

angular-automatic-lock-bot · 2019-11-21T16:10:45Z

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.}

googlebot added the cla: yes label Oct 17, 2019

CaerusKaru mentioned this pull request Oct 17, 2019

fix: add generic type for ModuleWithProviders angular/in-memory-web-api#245

Open

CaerusKaru force-pushed the adam/mwp branch 3 times, most recently from 4f96a13 to 5d38a20 Compare October 17, 2019 17:10

CaerusKaru marked this pull request as ready for review October 17, 2019 17:10

CaerusKaru requested review from a team as code owners October 17, 2019 17:11

vikerman requested review from devversion and kara October 17, 2019 18:43

kara suggested changes Oct 17, 2019

View reviewed changes

CaerusKaru changed the title ~~feat(core): remove fallback for ModuleWithProviders generic type~~ feat(core): add ModuleWithProviders generic type migration Oct 17, 2019

CaerusKaru force-pushed the adam/mwp branch from 5d38a20 to c04f6f2 Compare October 17, 2019 20:41

googlebot added cla: no and removed cla: yes labels Oct 17, 2019

CaerusKaru force-pushed the adam/mwp branch from c04f6f2 to ed52681 Compare October 17, 2019 20:46

googlebot added cla: yes and removed cla: no labels Oct 17, 2019

CaerusKaru force-pushed the adam/mwp branch from ed52681 to db2edeb Compare October 17, 2019 20:51

matsko added the area: core Issues related to the framework runtime label Oct 17, 2019

ngbot bot added this to the needsTriage milestone Oct 17, 2019

devversion reviewed Oct 18, 2019

View reviewed changes

CaerusKaru added 2 commits October 18, 2019 16:13

fixup! feat(core): add ModuleWithProviders generic type migration

eb94839

CaerusKaru force-pushed the adam/mwp branch 2 times, most recently from c41ee14 to 66ad830 Compare October 18, 2019 21:27

fixup! feat(core): add ModuleWithProviders generic type migration

fc6482f

CaerusKaru requested review from kara and devversion October 18, 2019 22:21

fixup! feat(core): add ModuleWithProviders generic type migration

9672579

CaerusKaru force-pushed the adam/mwp branch from cf39472 to 9672579 Compare October 18, 2019 22:48

devversion reviewed Oct 19, 2019

View reviewed changes

CaerusKaru force-pushed the adam/mwp branch 2 times, most recently from 9813cf9 to e3822ad Compare October 20, 2019 17:59

fixup! feat(core): add ModuleWithProviders generic type migration

dd4b96a

CaerusKaru force-pushed the adam/mwp branch from e3822ad to dd4b96a Compare October 20, 2019 18:06

kara approved these changes Oct 20, 2019

View reviewed changes

packages/core/schematics/migrations/module-with-providers/README.md Outdated Show resolved Hide resolved

packages/core/schematics/migrations/module-with-providers/transform.ts Outdated Show resolved Hide resolved

kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 20, 2019

kara assigned CaerusKaru Oct 20, 2019

kara added the target: major This PR is targeted for the next major release label Oct 20, 2019

fixup! feat(core): add ModuleWithProviders generic type migration

4e13fdc

devversion approved these changes Oct 21, 2019

View reviewed changes

devversion assigned kara and unassigned CaerusKaru Oct 21, 2019

kara approved these changes Oct 21, 2019

View reviewed changes

kara removed their assignment Oct 21, 2019

kara added action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 21, 2019

kara added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Oct 21, 2019

matsko closed this in 56731f6 Oct 21, 2019

CaerusKaru deleted the adam/mwp branch October 21, 2019 20:54

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): add ModuleWithProviders generic type migration #33217

feat(core): add ModuleWithProviders generic type migration #33217

CaerusKaru commented Oct 17, 2019 •

edited

Loading

CaerusKaru commented Oct 17, 2019

kara left a comment

googlebot commented Oct 17, 2019

googlebot commented Oct 17, 2019

devversion left a comment

kara left a comment

devversion left a comment

kara left a comment

kara commented Oct 21, 2019

kara commented Oct 21, 2019

angular-automatic-lock-bot bot commented Nov 21, 2019

feat(core): add ModuleWithProviders generic type migration #33217

feat(core): add ModuleWithProviders generic type migration #33217

Conversation

CaerusKaru commented Oct 17, 2019 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

CaerusKaru commented Oct 17, 2019

kara left a comment

Choose a reason for hiding this comment

googlebot commented Oct 17, 2019

googlebot commented Oct 17, 2019

devversion left a comment

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

devversion left a comment

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

kara commented Oct 21, 2019

kara commented Oct 21, 2019

angular-automatic-lock-bot bot commented Nov 21, 2019

CaerusKaru commented Oct 17, 2019 •

edited

Loading