test(ivy): create todo app in ivy #22921

mhevery · 2018-03-22T00:11:37Z

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

kara

LGTM

kara · 2018-03-22T19:15:01Z

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

+      - `bundle.min.js`: Is what the site would serve to their users. It has gone
+        through rollup, build-optimizer, and uglify with tree shaking.
+      - `bundle.min_debug.js`: Is what the developer would like to see when debugging
+        the application. It has also done through full pipeline of rollup, build-optimizer,


done -> gone

mhevery · 2018-03-22T19:16:34Z

tools/public_api_guard/core/core.d.ts

@@ -370,7 +370,8 @@ export declare const HostListener: HostListenerDecorator;

 /** @experimental */
 export declare function inject<T>(token: Type<T> | InjectionToken<T>, notFoundValue?: undefined, flags?: InjectFlags): T;
-export declare function inject<T>(token: Type<T> | InjectionToken<T>, notFoundValue: T | null, flags?: InjectFlags): T | null;
+export declare function inject<T>(token: Type<T> | InjectionToken<T>, notFoundValue: T, flags?: InjectFlags): T;


This change is needed to otherwise inject(Foo, new Foo())'s type is Foo|null which is not correct.

vicb · 2018-03-22T19:17:38Z

packages/compiler/src/render3/r3_view_compiler.ts

@@ -762,7 +751,7 @@ export function createFactory(
      unsupported('value dependencies');
    }
    if (dependency.isHost) {
-      unsupported('host dependencies');
+      // unsupported('host dependencies');


why ? add a comment ?

Opps, reverted

vicb · 2018-03-22T20:20:19Z

packages/compiler/src/render3/r3_view_compiler.ts

@@ -599,17 +599,6 @@ class TemplateDefinitionBuilder implements TemplateAstVisitor, LocalResolver {
            this._bindingMode, directive.sourceSpan, R3.elementProperty, o.literal(nodeIndex),
            o.literal(input.templateName), o.importExpr(R3.bind).callFn([convertedBinding]));
      }
-


Should those changes be part of a different commit describing the rationale for the change ?

broken to separate commits

vicb · 2018-03-22T20:22:38Z

packages/core/test/bundling/todo/BUILD.bazel

+    # path, to disambiguate from other workspaces.
+    # Here, the rule implementation is looking in an execroot where the layout
+    # has an "external" directory for external dependencies.
+    # This should probably start with "angular/" and let the rule deal with it.


# TODO: Remove once #22913 lands ?

#22913 is unrelated to this.

Well... that comes from you: #22654 (comment)

What did you mean then ?

resolved offline

vicb · 2018-03-22T20:23:38Z

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

+    {text: 'Demonstrate Components', done: false},
+    {text: 'Demonstrate Structural Directives', done: true},
+    {text: 'Demonstrate NgModules', done: false},
+    {text: 'Demonstrate zoneless changed detection', done: false},


typo "change"

vicb · 2018-03-22T20:24:58Z

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

+  // The reason for the guard is that template executes creation and binding together
+  // but NgForOf expects creation and binding separate.
+  template: `
+    <div [class.done]="todo && todo.done">


todo?.done ?

I intentionally left it in this ugly state because it is a hack which should be removed once we fix it in ivy runtime.

vicb · 2018-03-22T20:25:59Z

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

+  `
+})
+export class TodoComponent {
+  static DEFAULT_TODO: ToDo = {text: '', done: false};


remove ? (AppState.DEFAULT_TODO)

vicb · 2018-03-22T20:26:50Z

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

+
+  // TODO(misko): Injection is broken because the compiler generates incorrect code.
+  constructor(/**appState: AppState*/) {
+    // TODO(misko): remove once injection is working.


duplicate TODO ?

ngbot · 2018-03-22T23:31:45Z

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

IgorMinar

lgtm for api changes

but I'm surprised to see that we now have ivy_ng_module - fragmenting the build system like this seem like a bad idea. did you consult this with @alexeagle ?

mhevery · 2018-03-23T17:47:41Z

@IgorMinar Yes it is a bad idea. 🙂 It is internal only build rule/hack until we have proper rule https://github.com/angular/angular/blob/master/packages/compiler/design/separate_compilation.md#ng_module-output-bazel and yes this was discussed with @alexeagle @alxhub and @chuckjaz

ngbot · 2018-03-27T19:10:18Z

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

kara

LGTM

trotyl · 2018-03-30T02:29:47Z

packages/compiler/test/render3/r3_compiler_compliance_spec.ts

-              $r3$.ɵE(0, 'ul', null, null, $c1$);
-              $r3$.ɵC(2, $c2$, MyComponent_IfDirective_Template_2);
+              $r3$.ɵE(0, 'ul', null, $c1$);
+              $r3$.ɵC(2, MyComponent_IfDirective_Template_2, null, $c2$);


Since IfDirective is not determined at this stage any more, how could it still remains in the template function name? Same for MyComponent_ForOfDirective_Template_1 below.

You are right it should not be in the function name. We currently are still using old backend for the compiler which still has this information. The new compiler pipeline ngtsc will not and so when we switch over we will not be able to generate the string. Since the function name is there just to aid in debugging, i am going to leave it for now since it will get fixe automatically when we switch over to ngtsc.

trotyl · 2018-03-30T02:33:56Z

packages/core/src/render3/instructions.ts

@@ -400,7 +400,8 @@ function resetApplicationState() {
 * @param context to pass into the template.
 * @param providedRendererFactory renderer factory to use
 * @param host The host element node to use
- * @param defs Any directive or pipe defs that should be used for matching
+ * @param directives Any directive defs that should be used for matching
+ * @param pipes Any pipe defs that should be used for matching


Any directive defs -> Any directive types according to the change? (Should any be combined with plural?)

Removed the word any. Yes it is deys not types. We pass types to defineComponent which in turn converts them to defs. Because going from Type to Defs is megamorphic we want to do it as early as possible and than have all of the runtime be defs only.

ngbot · 2018-03-30T22:04:57Z

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

ngbot · 2018-04-02T21:26:23Z

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

Remove `containerRefreshStart` and `containerRefreshEnd` instruction from the output. Generate directives as a list in `componentDef` rather than inline into instructions. This is consistent in making selector resolution runtime so that translation of templates can follow locality.

PR Close #22921

Remove `containerRefreshStart` and `containerRefreshEnd` instruction from the output. Generate directives as a list in `componentDef` rather than inline into instructions. This is consistent in making selector resolution runtime so that translation of templates can follow locality. PR Close #22921

PR Close #22921

angular-automatic-lock-bot · 2019-09-13T11:52:23Z

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

marclaval mentioned this pull request Mar 22, 2018

bug(ivy): ngComponentDef.h is not a function #22922

Closed

matsko added the comp: ivy label Mar 22, 2018

mhevery force-pushed the bundle_todo_example branch from 7199883 to f1680b6 Compare March 22, 2018 17:48

mhevery added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Mar 22, 2018

kara approved these changes Mar 22, 2018

View reviewed changes

mhevery commented Mar 22, 2018

View reviewed changes

mhevery added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Mar 22, 2018

vicb reviewed Mar 22, 2018

View reviewed changes

ocombe added target: major This PR is targeted for the next major release and removed target: major This PR is targeted for the next major release labels Mar 22, 2018

vicb reviewed Mar 22, 2018

View reviewed changes

vicb approved these changes Mar 22, 2018

View reviewed changes

IgorMinar approved these changes Mar 23, 2018

View reviewed changes

mhevery force-pushed the bundle_todo_example branch 4 times, most recently from 6afcc32 to 65cf7d1 Compare March 27, 2018 00:10

mhevery force-pushed the bundle_todo_example branch 2 times, most recently from 2209b3d to 4e0511c Compare March 29, 2018 21:07

mhevery removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Mar 29, 2018

kara approved these changes Mar 29, 2018

View reviewed changes

trotyl reviewed Mar 30, 2018

View reviewed changes

mhevery force-pushed the bundle_todo_example branch from 84d3459 to 1e725ac Compare March 30, 2018 22:50

kara mentioned this pull request Mar 30, 2018

refactor(ivy): generate pipe names instead of defs #23104

Closed

mhevery added 5 commits April 2, 2018 14:28

refactor(ivy): cleanup necessary domino files

f6e6926

ci: update to latest rulse_nodejs

b3984f4

feat(ivy): add (event)="handle" syntax support to compiler

021d7b5

test(ivy): create todo app in ivy

248e8a0

mhevery force-pushed the bundle_todo_example branch from 1e725ac to 248e8a0 Compare April 2, 2018 21:36

alxhub closed this in e7f1af3 Apr 2, 2018

alxhub pushed a commit that referenced this pull request Apr 2, 2018

ci: update to latest rulse_nodejs (#22921)

4290ea4

PR Close #22921

alxhub pushed a commit that referenced this pull request Apr 2, 2018

feat(ivy): add (event)="handle" syntax support to compiler (#22921)

5266ffe

PR Close #22921

alxhub pushed a commit that referenced this pull request Apr 2, 2018

test(ivy): create todo app in ivy (#22921)

55c9fb2

PR Close #22921

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(ivy): create todo app in ivy #22921

test(ivy): create todo app in ivy #22921

mhevery commented Mar 22, 2018

kara left a comment

kara Mar 22, 2018

mhevery Mar 23, 2018

mhevery Mar 22, 2018

vicb Mar 22, 2018

mhevery Mar 22, 2018 •

edited

vicb Mar 22, 2018

mhevery Mar 23, 2018

vicb Mar 22, 2018

mhevery Mar 23, 2018

vicb Mar 26, 2018

mhevery Mar 29, 2018

vicb Mar 22, 2018

mhevery Mar 23, 2018

vicb Mar 22, 2018

mhevery Mar 23, 2018

vicb Mar 22, 2018

mhevery Mar 23, 2018

vicb Mar 22, 2018

mhevery Mar 23, 2018

ngbot bot commented Mar 22, 2018

IgorMinar left a comment

mhevery commented Mar 23, 2018

ngbot bot commented Mar 27, 2018

kara left a comment

trotyl Mar 30, 2018

mhevery Mar 30, 2018

trotyl Mar 30, 2018

mhevery Mar 30, 2018

ngbot bot commented Mar 30, 2018

ngbot bot commented Apr 2, 2018

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

test(ivy): create todo app in ivy #22921

test(ivy): create todo app in ivy #22921

Conversation

mhevery commented Mar 22, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

kara 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

Choose a reason for hiding this comment

mhevery Mar 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

ngbot bot commented Mar 22, 2018

IgorMinar left a comment

Choose a reason for hiding this comment

mhevery commented Mar 23, 2018

ngbot bot commented Mar 27, 2018

kara 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

Choose a reason for hiding this comment

ngbot bot commented Mar 30, 2018

ngbot bot commented Apr 2, 2018

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

mhevery Mar 22, 2018 •

edited