Bundle todo #23168

mhevery · 2018-04-04T16:57:04Z

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?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

pkozlowski-opensource · 2018-04-04T17:05:25Z

packages/compiler/src/render3/r3_view_compiler.ts

@@ -71,10 +71,14 @@ export function compileDirective(

  // e.g. `attributes: ['role', 'listbox']`
  field('attributes', createHostAttributesArray(directive, outputCtx));
+  console.error('=======', directive);


console.error

pkozlowski-opensource · 2018-04-04T17:10:23Z

packages/compiler/test/render3/r3_view_compiler_template_spec.ts

+/* These tests are codified version of the tests in compiler_canonical_spec.ts. Every
+  * test in compiler_canonical_spec.ts should have a corresponding test here.
+  */
+fdescribe('compiler compliance: listen()', () => {


fdescribe

pkozlowski-opensource · 2018-04-04T17:16:21Z

packages/core/test/bundling/todo/OUTSTANDING_WORK.md

 ## Compiler
 - [ ] Remove ` tslib_1.__decorate([core_1.Input(), tslib_1.__metadata("design:type", Object)], TodoComponent.prototype, "todo", void 0);` from generated output.
-
+- [ ] Allow compilation of `@angular/common` through ivy.
+
 ## Ivy Runtime
 - [ ] Work on `ViewContainerRef` needs to cause change detection so that `todo` app renders correctly on first render.
 - [ ] The todo input value box is not correctly rendering to checked for completed tasks.


This should be fixed by #23161, right?

buildsize · 2018-04-04T20:45:56Z

File name	Previous Size	New Size	Change
bundle.min.js	52.9 KB	39.88 KB	-13.02 KB (25%)
bundle.min.js.brotli	14.18 KB	11.26 KB	-2.92 KB (21%)

vicb · 2018-04-04T21:09:34Z

packages/core/src/render3/instructions.ts

@@ -1765,7 +1765,7 @@ export function wrapListenerWithDirtyLogic(view: LView, listenerFn: (e?: any) =>
 */
 export function wrapListenerWithDirtyAndDefault(
    view: LView, listenerFn: (e?: any) => any): EventListener {
-  return function(e: Event) {
+  return function wrapListenerIn_markViewDirty(e: Event) {


follow CS wrapListenerInMarkViewDirty ?

I would like to keep this as is unless you feel strongly. Rest of the generated code follows this convention such as ToDoAppComponent_NgIf_NgForOf_NgIf_Template_6

vicb · 2018-04-04T21:11:01Z

packages/compiler/src/render3/r3_view_compiler.ts

+  return null;
+}
+
+function createOutputsObject(


factorize createInputsObject / createOutputsObject ?

vicb · 2018-04-04T21:12:16Z

packages/compiler/test/render3/r3_view_compiler_input_outputs_spec.ts

+              })
+              export class MyComponent {
+                @Input() name;
+                @Input('renamed') original;


@Input() input; @Input('renamedInput') originalInput;

?

vicb · 2018-04-04T21:13:57Z

packages/compiler/test/render3/r3_view_compiler_input_outputs_spec.ts

+                selector: '[my-directive]',
+              })
+              export class MyDirective {
+                @Input() dirName;


same comment as for the cmp: consistency for I vs O names

vicb · 2018-04-04T21:15:04Z

packages/compiler/test/render3/r3_view_compiler_input_outputs_spec.ts

+import {compile, expectEmit} from './mock_compile';
+
+/* These tests are codified version of the tests in compiler_canonical_spec.ts. Every
+  * test in compiler_canonical_spec.ts should have a corresponding test here.


is it the case ?

removed comment

vicb · 2018-04-04T21:16:37Z

Commit message typo -> "statmentes" (es) -> "statements" (en)

vicb · 2018-04-04T21:19:32Z

packages/compiler/test/render3/r3_view_compiler_template_spec.ts

+    const template = `
+      template:function MyComponent_Template(ctx:any,cm:boolean){
+        if (cm) { 
+          i0.ɵC(0,MyComponent_NgForOf_Template_0,null,_c0); 


MyComponent_NgForOf_Template_0, _c0, i0, ... should not be hardcoded
use IDENT or $matchingName$ instead

vicb · 2018-04-04T21:20:07Z

packages/compiler/test/render3/r3_view_compiler_template_spec.ts

+import {MockDirectory, setup} from '../aot/test_util';
+import {compile, expectEmit} from './mock_compile';
+
+/* These tests are codified version of the tests in compiler_canonical_spec.ts. Every


not the case ?

vicb

See inline comments in each commits

kara · 2018-04-05T21:08:03Z

packages/compiler/test/render3/r3_view_compiler_template_spec.ts

+              if (cm) {
+                i0.ɵE(0,'div');
+                i0.ɵL('click',function MyComponent_NgForOf_NgForOf_NgForOf_Template_1_div_click_listener($event:any){
+                  const pd_b:any = ((<any>ctx.onClick($_r0$,$_r1$,$_r2$)) !== false);


This generated code doesn't match our canonical spec for listeners here. Is there a reason for that or is this just work that hasn't been done yet in the compiler?

We should convert the canonical specs to this format. Out of scope for this PR.

I have updated the generated code to match spec. It is a bit of a hack so please let take a look if you are OK with it.

ngbot · 2018-04-05T21:58:03Z

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

kara

LGTM

mary-poppins · 2018-04-06T23:16:45Z

You can preview 4560326 at https://pr23168-4560326.ngbuilds.io/.
You can preview 182c47f at https://pr23168-182c47f.ngbuilds.io/.
You can preview d64fa70 at https://pr23168-d64fa70.ngbuilds.io/.
You can preview 8ab9ba1 at https://pr23168-8ab9ba1.ngbuilds.io/.

mary-poppins · 2018-04-06T23:16:45Z

You can preview 4579242 at https://pr23168-4579242.ngbuilds.io/.

vicb · 2018-04-06T23:20:00Z

packages/compiler/src/render3/r3_view_compiler.ts

+function noop() {}
+
+/**
+ * Function which is executed whenever a variable is refferenced for the first time in a given


vicb · 2018-04-06T23:21:22Z

packages/compiler/src/render3/r3_view_compiler.ts

+   * - value `lhs` is the left hand side which is an AST representing `abc` which
+   *   can be used in other places of the source code.
+   * - value `rhs` is the right hand side which is an AST representing `a.b.c` which
+   *   can be used in other places of the source code.


not sure to understand "which can be used in other places of the source code." ?

That was a left over from previous behavior. I have removed it and I think it makes it clearer.

vicb · 2018-04-06T23:25:10Z

packages/compiler/test/render3/r3_view_compiler_input_outputs_spec.ts

+
+    // The template should look like this (where IDENT is a wild card for an identifier):
+    const template = `
+      static ngComponentDef = i0.ɵdefineComponent({


i0 should not be hardcoded

change to IDENT

vicb · 2018-04-06T23:25:25Z

packages/compiler/test/render3/r3_view_compiler_input_outputs_spec.ts

+      }
+    };
+
+    // The template should look like this (where IDENT is a wild card for an identifier):


there is no IDENT

now there is

vicb · 2018-04-06T23:26:00Z

packages/compiler/test/render3/r3_view_compiler_input_outputs_spec.ts

@@ -0,0 +1,86 @@
+/**


looks like a conformance test ? should it be moved to that folder ?

Why do you think it is conformance. I think of it as unit test for the r3_view_compiler.ts but that test file is huge so i suffixed it by type intpu_outputs.

Why do you think it is conformance

Because the only assertion you have here is about the compiler output.
Probably we should have a clear definition of the types of test and what they should do.

discussed in person

vicb · 2018-04-09T17:51:16Z

packages/compiler/src/render3/r3_view_compiler.ts

@@ -193,7 +196,10 @@ export function compileComponent(
  }

  // e.g `inputs: {a: 'a'}`
-  field('inputs', createInputsObject(component, outputCtx));
+  field('inputs', createMapObjectLiteral(component.inputs, outputCtx));


Other fields are defined only when they should (ie features, directives, pipes, ...).
Should we do the same for inputs and outputs ?
It would be better for code size.

discussed in person, it is OK.

vicb · 2018-04-09T18:22:29Z

packages/core/test/bundling/todo/index.ts

+  set title(value: string) { this._title = value.trim(); }
+
+  constructor(title: string, completed: boolean = false) {
+    this.completed = completed;


public completed = false in the args ?

vicb · 2018-04-09T18:22:43Z

packages/core/test/bundling/todo/index.ts

+  constructor(title: string, completed: boolean = false) {
+    this.completed = completed;
+    this.editing = false;
+    this.title = title.trim();


No need to trim here (done in the setter)

…nent`

Given ``` <div *ngFor=”…” (click)=“doSomething()”> ``` Before `doSomething` would execute on the inner template context, which is incorrect. The correct behavior is to execute on the top level context of the component.

When compiling templates the compiler would often bind to closest context rather than the component context. The only time one should be binding to the cont component is in explicit cases where the inner template declares local variable.

mary-poppins · 2018-04-09T23:08:57Z

You can preview 2311600 at https://pr23168-2311600.ngbuilds.io/.

mary-poppins · 2018-04-09T23:18:28Z

You can preview f2e55de at https://pr23168-f2e55de.ngbuilds.io/.

PR Close #23168

…nent` (#23168) PR Close #23168

Given ``` <div *ngFor=”…” (click)=“doSomething()”> ``` Before `doSomething` would execute on the inner template context, which is incorrect. The correct behavior is to execute on the top level context of the component. PR Close #23168

…#23168) PR Close #23168

PR Close #23168

When compiling templates the compiler would often bind to closest context rather than the component context. The only time one should be binding to the cont component is in explicit cases where the inner template declares local variable. PR Close #23168

PR Close #23168

angular-automatic-lock-bot · 2019-09-13T12:51:22Z

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 Apr 4, 2018

pkozlowski-opensource reviewed Apr 4, 2018

View reviewed changes

mhevery force-pushed the bundle_todo branch from 4560326 to 182c47f Compare April 4, 2018 18:52

mhevery changed the title ~~WIP: Bundle todo~~ Bundle todo Apr 4, 2018

mhevery force-pushed the bundle_todo branch from d64fa70 to 8ab9ba1 Compare April 4, 2018 20:35

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

vicb reviewed Apr 4, 2018

View reviewed changes

vicb suggested changes Apr 4, 2018

View reviewed changes

alxhub added the comp: ivy label Apr 4, 2018

kara reviewed Apr 5, 2018

View reviewed changes

mhevery force-pushed the bundle_todo branch from e6d11be to 4579242 Compare April 6, 2018 23:07

kara approved these changes Apr 6, 2018

View reviewed changes

vicb reviewed Apr 6, 2018

View reviewed changes

vicb reviewed Apr 9, 2018

View reviewed changes

mhevery added 6 commits April 9, 2018 15:40

docs(ivy): update outstanding work for todo

7d5b361

fix(ivy): named listener method for easier debugging.

76c670a

feat(ivy): Add outputs support for defineDirective / `defineCompo…

50bf51b

…nent`

build: create a more focused build file for packages/compiler/render3

ab5cb1d

test(ivy): cleanup todo test app

aeb8552

mhevery force-pushed the bundle_todo branch from f5e88ef to 2311600 Compare April 9, 2018 22:42

vicb approved these changes Apr 9, 2018

View reviewed changes

mhevery added 3 commits April 9, 2018 16:05

fix(ivy): correctly bind to component context

34a918d

When compiling templates the compiler would often bind to closest context rather than the component context. The only time one should be binding to the cont component is in explicit cases where the inner template declares local variable.

docs(ivy): update STATUS.md with outstanding work

1049797

test(ivy): update todo app to http://todomvc.com

f2e55de

mhevery force-pushed the bundle_todo branch from 2311600 to f2e55de Compare April 9, 2018 23:05

mhevery added the action: merge The PR is ready for merge by the caretaker label Apr 10, 2018

IgorMinar closed this in 720031b Apr 10, 2018

IgorMinar pushed a commit that referenced this pull request Apr 10, 2018

fix(ivy): named listener method for easier debugging. (#23168)

9a2479d

PR Close #23168

IgorMinar pushed a commit that referenced this pull request Apr 10, 2018

feat(ivy): Add outputs support for defineDirective / `defineCompo…

c059670

…nent` (#23168) PR Close #23168

IgorMinar pushed a commit that referenced this pull request Apr 10, 2018

build: create a more focused build file for packages/compiler/render3 (…

50ab8d8

…#23168) PR Close #23168

IgorMinar pushed a commit that referenced this pull request Apr 10, 2018

test(ivy): cleanup todo test app (#23168)

4f7fac0

PR Close #23168

IgorMinar pushed a commit that referenced this pull request Apr 10, 2018

docs(ivy): update STATUS.md with outstanding work (#23168)

50030f6

PR Close #23168

IgorMinar pushed a commit that referenced this pull request Apr 10, 2018

test(ivy): update todo app to http://todomvc.com (#23168)

f1db789

PR Close #23168

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

Bundle todo #23168

Bundle todo #23168

Conversation

mhevery commented Apr 4, 2018

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

buildsize bot commented Apr 4, 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

vicb Apr 4, 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

vicb commented Apr 4, 2018

vicb Apr 4, 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

vicb left a 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 Apr 5, 2018

kara left a comment

Choose a reason for hiding this comment

mary-poppins commented Apr 6, 2018

mary-poppins commented Apr 6, 2018

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

mary-poppins commented Apr 9, 2018

mary-poppins commented Apr 9, 2018

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

buildsize bot commented Apr 4, 2018 •

edited

vicb Apr 4, 2018 •

edited

vicb Apr 4, 2018 •

edited