Back patch spec #22235

mhevery · 2018-02-15T01:59:03Z

This PR is a WIP which provides an example how Ivy and non-Ivy code can interoperate through back patching.

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
[X] Other... Please describe: Compiler canonical design spec

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

trotyl · 2018-02-15T05:31:21Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+export class LibAModule {}
+// END FILE: node_modules/libA/module.ts
+// BEGIN FILE: node_modules/libA/module.metadata.json
+// Abridged version of metadata


A bridged (space between)?

_{Another case below.}

:-) Actually abridged is a word

trotyl · 2018-02-15T05:34:01Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+const node_modules_libA_module_metadata = {
+  'LibAModule': { 
+    refs: ["LibAComponent"], 
+    constructorDes: [],


Des? Deps?

_{Several other cases below.}

trotyl · 2018-02-15T05:45:31Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+// END FILE: src/main.ts
+
+// BEGIN FILE: src/app.ngfactory.ts
+function ngBackPatch_node_modules_libB_module() {


Should it return true? The returned value is used in logical and below.

We should update the code not to rely on the result of this function.

It is important that it be easy to remove everything from this function so what it be empty after the build optimizer is finished with it.

added : void

trotyl · 2018-02-15T05:46:36Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+  moduleType: AppModule,
+  patchedDeps: false,
+  create(parentInjector: Injector | null): NgModuleRef<AppModule> {
+    this.patchedDeps && ngBackPatch_node_modules_libB_module() && (this.patchedDeps = true);


!this.patchedDeps && ... rather than this.patchedDeps && ...?

We should generate this more simply as,

if (!this.patchDeps) { this.patchedDeps = true; ngBackPatch_node_modules_libB_module(); }

which will be rewritten by uglify into something like:

!this.patchedDeps && (this.patchedDeps = true, b())

In other words, lets leave the ugly code production to uglify.

As it shouldn't matter, it is slightly safer to set this.pachedDeps = true before calling the back-patch function which would an indirect recursive loop if one exists, so I moved the set of patchedDeps.

trotyl · 2018-02-15T05:52:17Z

packages/core/test/render3/compiler_canonical/patch_types_spec.ts

+class CompiledWithIvy {
+  // NORMATIVE
+  static ngInjectableDef = pending_pull_22005.defineInjectable(
+      {factory: function CompileWithIvy_Factory() { return new CompiledWithIvy(); }});


CompileWithIvy_Factory -> CompiledWithIvy_Factory according to the class name?

trotyl · 2018-02-15T05:56:33Z

packages/core/test/render3/compiler_canonical/patch_types_spec.ts

+function ngPatch_node_modules_some_library_path_public_CompileWithIvy() {
+  /** @__BUILD_OPTIMIZER_COLOCATE__ */
+  (ThirdPartyClass as any).ngInjectableDef = pending_pull_22005.defineInjectable(
+      {factory: function CompileWithIvy_Factory() { return new ThirdPartyClass(); }});


CompileWithIvy_Factory -> ThirdPartyClass_Factory?

ngbot · 2018-02-21T00:35:08Z

Hi @mhevery! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

chuckjaz · 2018-02-22T21:49:23Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+  moduleType: AppModule,
+  patchedDeps: false,
+  create(parentInjector: Injector | null): NgModuleRef<AppModule> {
+    this.patchedDeps && ngBackPatch_node_modules_libB_module() && (this.patchedDeps = true);


We should generate this more simply as,

if (!this.patchDeps) { this.patchedDeps = true; ngBackPatch_node_modules_libB_module(); }

which will be rewritten by uglify into something like:

!this.patchedDeps && (this.patchedDeps = true, b())

In other words, lets leave the ugly code production to uglify.

As it shouldn't matter, it is slightly safer to set this.pachedDeps = true before calling the back-patch function which would an indirect recursive loop if one exists, so I moved the set of patchedDeps.

chuckjaz · 2018-02-22T22:10:18Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+}
+
+export const AppModuleFactory: NgModuleFactory<AppModule>&{patchedDeps: boolean} =  {
+  moduleType: AppModule,


FYI: This is generated in a separate module from AppModule so we import this using i0.AppModule syntax so the normative section should reflect that.

chuckjaz · 2018-02-22T22:11:51Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+
+export const AppModuleFactory: NgModuleFactory<AppModule>&{patchedDeps: boolean} =  {
+  moduleType: AppModule,
+  patchedDeps: false,


We don't actually need this as the default value of patchDeps is undefined. We should consider changing the declaration to ... & {patchedDeps?: true} and remove this explicit set to false.

Good idea, Done.

chuckjaz · 2018-02-22T22:13:43Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+export const AppModuleFactory: NgModuleFactory<AppModule>&{patchedDeps: boolean} =  {
+  moduleType: AppModule,
+  patchedDeps: false,
+  create(parentInjector: Injector | null): NgModuleRef<AppModule> {


This syntax is hard for me to create. Consider changing this to a function expression create: function AppModuleFactory_Create { ... }

Produces back-patch as described in the angular#22235 and referenced in angular#22480. This just contains the compiler implementations and the corresponding unit tests. Connecting the dots as described in angular#22480 will be in a follow on change.

chuckjaz · 2018-03-01T00:26:48Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+
+// Simulate Imports
+const i0 = {
+  AppModule: MyAppModule,


MyAppModule: MyAppModule?

chuckjaz · 2018-03-01T00:27:50Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+  ngBackPatch_node_modules_libB_module_packageAComponent();
+  ngBackPatch_node_modules_libB_module_packageAModule();
+}
+


// NORMATIVE

chuckjaz · 2018-03-01T00:28:01Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+function ngBackPatch_node_modules_libB_module_packageAModule(): void {
+  (NpmModule as any).ngInjectorDef = pending_pull_22458.defineInjector(details_elided);
+}
+


// /NORMATIVE

chuckjaz · 2018-03-01T00:28:32Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+const i0 = {
+  AppModule: MyAppModule,
+};
+


// NORMATIVE

chuckjaz · 2018-03-01T00:28:47Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+          this.patchedDeps = true;
+          ngBackPatch_node_modules_libB_module();
+        }
+        return details_elided;


// /NORMATIVE

chuckjaz · 2018-03-01T00:29:24Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+}
+
+// Simulate Imports
+const i0 = {


Please change i0 to $i0$

chuckjaz · 2018-03-01T00:30:38Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+
+export const AppModuleFactory: NgModuleFactory<MyAppModule>&{patchedDeps?: true} = {
+  moduleType: i0.AppModule,
+  create: function AppModuleFactory_Create(parentInjector: Injector | null):


This would be easier if you created:

type $ParentInjector$ = Injector | null;

and used that here. This allows me to cut/paste this directly instead of having to fix up the types.

chuckjaz · 2018-03-01T00:32:27Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+import {Component, ContentChild, Directive, Injectable, Injector, Input, NgModule, OnDestroy, Optional, Pipe, PipeTransform, QueryList, SimpleChanges, TemplateRef, Type, ViewChild, ViewContainerRef} from '../../../src/core';
+import * as r3 from '../../../src/render3/index';
+
+import {pending_pull_22458} from './small_app_spec';


import {pending_pull_22458 as $pending_pull_22458$} from './small_app_spec';

chuckjaz · 2018-03-01T00:34:29Z

packages/core/test/render3/compiler_canonical/back_patch_types_specs.ts

+import {NgModuleFactory, NgModuleRef} from 'core';
+
+import {Component, ContentChild, Directive, Injectable, Injector, Input, NgModule, OnDestroy, Optional, Pipe, PipeTransform, QueryList, SimpleChanges, TemplateRef, Type, ViewChild, ViewContainerRef} from '../../../src/core';
+import * as r3 from '../../../src/render3/index';


import * as $r3$ from '../../src/core_rener3_private_export'; and then use the ɵ versions.

Produces back-patch as described in the angular#22235 and referenced in angular#22480. This just contains the compiler implementations and the corresponding unit tests. Connecting the dots as described in angular#22480 will be in a follow on change.

ngbot · 2018-03-02T23:50:56Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     missing required label: "PR target: *"
     status "google3" is pending
     status "ci/circleci: lint" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

mhevery · 2018-03-03T18:13:30Z

http://test/OCL:187738682:BASE:187738680:1520100801542:fa96f1e6

Produces back-patch as described in the angular#22235 and referenced in angular#22480. This just contains the compiler implementations and the corresponding unit tests. Connecting the dots as described in angular#22480 will be in a follow on change.

Produces back-patch as described in the #22235 and referenced in #22480. This just contains the compiler implementations and the corresponding unit tests. Connecting the dots as described in #22480 will be in a follow on change. PR Close #22506

PR Close angular#22235

) Produces back-patch as described in the angular#22235 and referenced in angular#22480. This just contains the compiler implementations and the corresponding unit tests. Connecting the dots as described in angular#22480 will be in a follow on change. PR Close angular#22506

angular-automatic-lock-bot · 2019-09-13T09:52:16Z

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

mhevery requested a review from chuckjaz February 15, 2018 01:59

googlebot added the cla: yes label Feb 15, 2018

mhevery added this to Sprint Backlog in Framework via automation Feb 15, 2018

mhevery force-pushed the back_patch branch from 75bf19b to 70ab646 Compare February 15, 2018 03:18

trotyl reviewed Feb 15, 2018

View reviewed changes

vicb added the area: core Issues related to the framework runtime label Feb 15, 2018

mhevery moved this from Sprint Backlog to In progress in Framework Feb 16, 2018

mhevery force-pushed the back_patch branch from 70ab646 to 9d78240 Compare February 17, 2018 01:32

chuckjaz suggested changes Feb 22, 2018

View reviewed changes

chuckjaz reviewed Feb 22, 2018

View reviewed changes

mhevery moved this from In progress to in review in Framework Feb 28, 2018

mhevery moved this from in review to In progress in Framework Feb 28, 2018

mhevery force-pushed the back_patch branch from 9d78240 to 08b55e3 Compare February 28, 2018 22:59

chuckjaz mentioned this pull request Feb 28, 2018

feat(ivy): produce Renderer2 back-patching and factories. #22506

Closed

mhevery force-pushed the back_patch branch from 08b55e3 to 2838aa0 Compare March 1, 2018 00:11

mhevery added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 1, 2018

mhevery force-pushed the back_patch branch from 2838aa0 to 7fdd69e Compare March 1, 2018 00:14

chuckjaz suggested changes Mar 1, 2018

View reviewed changes

mhevery changed the title ~~WIP: Back patch~~ Back patch spec Mar 1, 2018

mhevery force-pushed the back_patch branch 2 times, most recently from 1aa4500 to b437bbc Compare March 1, 2018 17:08

chuckjaz approved these changes Mar 2, 2018

View reviewed changes

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 Mar 2, 2018

test(ivy): Back patch example

97e8362

mhevery force-pushed the back_patch branch from b437bbc to 97e8362 Compare March 3, 2018 18:04

mhevery added the target: major This PR is targeted for the next major release label Mar 3, 2018

mhevery force-pushed the back_patch branch from ca4888a to 615e7b7 Compare March 5, 2018 04:29

fixup! test(ivy): Back patch example

b433524

mhevery force-pushed the back_patch branch from 615e7b7 to b433524 Compare March 5, 2018 04:37

alexeagle closed this in 363dfa5 Mar 6, 2018

Framework automation moved this from In progress to Done / Merged Mar 6, 2018

mhevery removed this from Done / Merged in Framework Mar 13, 2018

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

test(ivy): Back patch example (angular#22235)

31dd243

PR Close angular#22235

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

Back patch spec #22235

Back patch spec #22235

Conversation

mhevery commented Feb 15, 2018 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckjaz Feb 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngbot bot commented Feb 21, 2018

chuckjaz Feb 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngbot bot commented Mar 2, 2018

mhevery commented Mar 3, 2018

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

mhevery commented Feb 15, 2018 •

edited

chuckjaz Feb 22, 2018 •

edited

chuckjaz Feb 22, 2018 •

edited