fix(ivy): check semantics of NgModule for consistency #27604

mhevery · 2018-12-11T18:43:59Z

NgModule requires that Components/Directives/Pipes are listed in
declarations, and that each Components/Directives/Pipe is declared
in exactly one NgModule. This change adds runtime checks to ensure
that these sementics are true at runtime.

There will need to be seperate set of checks for the AoT path of the
codebase to verify that same set of semantics hold. Due to current
design there does not seem to be an easy way to share the two checks
because JIT deal with references where as AoT deals with AST nodes.

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)
[n/a] 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:

Does this PR introduce a breaking change?

Yes
No

mary-poppins · 2018-12-11T18:48:10Z

You can preview 8197b6d at https://pr27604-8197b6d.ngbuilds.io/.

kara

Nits

packages/core/test/linker/ng_module_integration_spec.ts

packages/core/src/linker/ng_module_factory_loader.ts

packages/core/src/render3/jit/module.ts

alxhub · 2018-12-11T19:15:31Z

packages/core/src/linker/ng_module_factory_loader.ts

 }

-export function clearModulesForTest() {
-  moduleFactories = new Map<string, NgModuleFactory<any>>();
+export function registerNgModuleType(id: string, ngModuleType: NgModuleType) {


This could have been a separate feat commit, since this is new behavior :)

packages/core/test/linker/ng_module_integration_spec.ts

packages/core/src/linker/ng_module_factory_loader.ts

packages/core/src/di/r3_injector.ts

packages/core/src/linker/ng_module_factory_loader.ts

packages/core/src/render3/jit/module.ts

packages/core/test/linker/ng_module_integration_spec.ts

packages/core/testing/src/r3_test_bed.ts

googlebot · 2018-12-11T21:47:48Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-12-11T21:47:57Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-12-11T21:47:57Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-12-11T21:47:58Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

mary-poppins · 2018-12-11T21:50:08Z

You can preview c0b8828 at https://pr27604-c0b8828.ngbuilds.io/.

mary-poppins · 2018-12-11T21:50:30Z

You can preview 09243f9 at https://pr27604-09243f9.ngbuilds.io/.

mary-poppins · 2018-12-11T21:50:44Z

You can preview 5ccfe99 at https://pr27604-5ccfe99.ngbuilds.io/.

mary-poppins · 2018-12-11T21:51:33Z

You can preview e4614ce at https://pr27604-e4614ce.ngbuilds.io/.

mary-poppins · 2018-12-11T21:52:01Z

You can preview 328d4e9 at https://pr27604-328d4e9.ngbuilds.io/.

mary-poppins · 2018-12-11T21:52:01Z

You can preview 33668b4 at https://pr27604-33668b4.ngbuilds.io/.

mary-poppins · 2018-12-11T21:52:16Z

You can preview 2ba9011 at https://pr27604-2ba9011.ngbuilds.io/.

mary-poppins · 2018-12-13T22:24:45Z

You can preview 5aaea5e at https://pr27604-5aaea5e.ngbuilds.io/.

mary-poppins · 2018-12-14T00:34:51Z

You can preview b8bf96c at https://pr27604-b8bf96c.ngbuilds.io/.

kara

LGTM, aside from the CI failures

googlebot · 2018-12-14T22:10:38Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

mary-poppins · 2018-12-14T22:12:01Z

You can preview 679ff56 at https://pr27604-679ff56.ngbuilds.io/.

`NgModule` requires that `Component`s/`Directive`s/`Pipe`s are listed in declarations, and that each `Component`s/`Directive`s/`Pipe` is declared in exactly one `NgModule`. This change adds runtime checks to ensure that these sementics are true at runtime. There will need to be seperate set of checks for the AoT path of the codebase to verify that same set of semantics hold. Due to current design there does not seem to be an easy way to share the two checks because JIT deal with references where as AoT deals with AST nodes.

googlebot · 2018-12-15T00:37:41Z

CLAs look good, thanks!

mary-poppins · 2018-12-15T00:41:43Z

You can preview 30e5f7f at https://pr27604-30e5f7f.ngbuilds.io/.

mary-poppins · 2018-12-17T17:59:23Z

You can preview 5ce177a at https://pr27604-5ce177a.ngbuilds.io/.

mhevery · 2018-12-17T18:25:46Z

http://test/OCL:225848211:BASE:225866837:1545077311378:3d5ea958

mary-poppins · 2018-12-17T20:11:04Z

You can preview 66e2f6f at https://pr27604-66e2f6f.ngbuilds.io/.

`NgModule` requires that `Component`s/`Directive`s/`Pipe`s are listed in declarations, and that each `Component`s/`Directive`s/`Pipe` is declared in exactly one `NgModule`. This change adds runtime checks to ensure that these sementics are true at runtime. There will need to be seperate set of checks for the AoT path of the codebase to verify that same set of semantics hold. Due to current design there does not seem to be an easy way to share the two checks because JIT deal with references where as AoT deals with AST nodes. PR Close angular#27604

angular-automatic-lock-bot · 2019-09-14T10:53:12Z

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 Dec 11, 2018

mhevery requested a review from alxhub December 11, 2018 18:44

mhevery assigned kara Dec 11, 2018

kara requested changes Dec 11, 2018

View reviewed changes

alxhub approved these changes Dec 11, 2018

View reviewed changes

kara added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Dec 11, 2018

ngbot bot added this to the needsTriage milestone Dec 11, 2018

kara removed their assignment Dec 11, 2018

JoostK reviewed Dec 11, 2018

View reviewed changes

googlebot added cla: no and removed cla: yes labels Dec 11, 2018

mhevery added effort2: days risk: low labels Dec 11, 2018

ngbot bot modified the milestones: needsTriage, Backlog Dec 11, 2018

mhevery force-pushed the fw-682 branch from 341462c to 5aaea5e Compare December 13, 2018 22:20

kara approved these changes Dec 14, 2018

View reviewed changes

mhevery force-pushed the fw-682 branch from b8bf96c to 679ff56 Compare December 14, 2018 22:07

mhevery added cla: yes and removed cla: no labels Dec 14, 2018

mhevery added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 14, 2018

mhevery added 2 commits December 14, 2018 15:44

fixup! fix(ivy): check semantics of NgModule for consistency

a979f5f

mhevery force-pushed the fw-682 branch from 679ff56 to 30e5f7f Compare December 15, 2018 00:37

mhevery added 2 commits December 17, 2018 09:53

fixup! fix(ivy): check semantics of NgModule for consistency

f17b6d0

fixup! fix(ivy): check semantics of NgModule for consistency

5ce177a

mhevery force-pushed the fw-682 branch from 30e5f7f to 5ce177a Compare December 17, 2018 17:54

fixup! fix(ivy): check semantics of NgModule for consistency

66e2f6f

mhevery closed this in e94975d Dec 17, 2018

kara mentioned this pull request Dec 18, 2018

test(ivy): replace resolved issue in compiler-cli test with new root … #27709

Closed

This was referenced Dec 19, 2018

test(ivy): all NgModule should be compiled #27767

Closed

test(ivy): add root cause analysis for failing router tests #27792

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ivy): check semantics of NgModule for consistency #27604

fix(ivy): check semantics of NgModule for consistency #27604

mhevery commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

kara left a comment

alxhub Dec 11, 2018

googlebot commented Dec 11, 2018

googlebot commented Dec 11, 2018

googlebot commented Dec 11, 2018

googlebot commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 13, 2018

mary-poppins commented Dec 14, 2018

kara left a comment •

edited

googlebot commented Dec 14, 2018

mary-poppins commented Dec 14, 2018

googlebot commented Dec 15, 2018

mary-poppins commented Dec 15, 2018

mary-poppins commented Dec 17, 2018

mhevery commented Dec 17, 2018 •

edited

mary-poppins commented Dec 17, 2018

angular-automatic-lock-bot bot commented Sep 14, 2019

fix(ivy): check semantics of NgModule for consistency #27604

fix(ivy): check semantics of NgModule for consistency #27604

Conversation

mhevery commented Dec 11, 2018

PR Checklist

PR Type

Does this PR introduce a breaking change?

mary-poppins commented Dec 11, 2018

kara left a comment

Choose a reason for hiding this comment

alxhub Dec 11, 2018

Choose a reason for hiding this comment

googlebot commented Dec 11, 2018

googlebot commented Dec 11, 2018

googlebot commented Dec 11, 2018

googlebot commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 11, 2018

mary-poppins commented Dec 13, 2018

mary-poppins commented Dec 14, 2018

kara left a comment • edited

Choose a reason for hiding this comment

googlebot commented Dec 14, 2018

mary-poppins commented Dec 14, 2018

googlebot commented Dec 15, 2018

mary-poppins commented Dec 15, 2018

mary-poppins commented Dec 17, 2018

mhevery commented Dec 17, 2018 • edited

mary-poppins commented Dec 17, 2018

angular-automatic-lock-bot bot commented Sep 14, 2019

kara left a comment •

edited

mhevery commented Dec 17, 2018 •

edited