Sprint3 #22018

mhevery · 2018-02-05T04:23:11Z

PR Checklist

Please check if your PR fulfills the following requirements:

write unit test for markDirty
write unit test for detectChanges
write unit test for whenRendered
write unit test for withBody
write unit test and docs for ensureDocument
write unit test and docs for cleanupDocument
~~update patch_types_spec.ts with discussion per @chuckjaz~~ (separate CL)
rework hell world bundle tests to use withBody

@IgorMinar & @chuckjaz Please check out patch_types_spec.ts if this is compatible with your understanding of the world.

trotyl · 2018-02-05T05:17:54Z

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

+/**
+ * GOALS:
+ * - Patch types in tree shakable way
+ * - Generate these types for files which have `matadatad.json` (since those are the files which


matadatad.json? Typo?

trotyl · 2018-02-05T06:31:03Z

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

+        await r3.whenRendered(todoApp);
+        expect(r3.getRenderedText(todoApp)).toEqual('...');
+        const firstCheckBox =
+            r3.getHostElement(todoApp).querySelector('input[type]=checkbox') as HTMLElement;


Broken selector?

JoostK · 2018-02-05T20:29:17Z

packages/core/src/render3/component.ts

+ * Use this function to bootstrap a component into the DOM tree. Each invocation
+ * of this function will create a separate tree of components, injectors and
+ * change detection cycles and lifetimes. To dynamically insert a new component into
+ * existing tree such that it share the same injection, change detection and object


To dynamically insert a new component into an existing tree such that it shares the same injection

JoostK · 2018-02-05T20:30:50Z

packages/core/src/render3/component.ts

+ * at some future point in time. This is because a single user action often results in many
+ * components being invalidated and calling change detection on each component synchronously
+ * would be inefficient. It is better to wait until all components re marked as dirty and
+ * than perform single change detection across all of the components.


until all components are marked as dirty and then perform

JoostK · 2018-02-05T20:36:02Z

packages/core/src/render3/component.ts

+export function markDirty<T>(component: T) {
+  const rootContext = getRootContext(component);
+  let res: null|((val: null) => void);
+  if (!(res = rootContext.cleanResolve)) {


res shouldn't have to be assigned here, as its value is not used. In fact, res is only used within the if.

JoostK · 2018-02-05T20:38:07Z

packages/core/src/render3/component.ts

+ * @param component Component for which the host element should be retrieved.
+ */
+export function getHostElement<T>(component: T): HTMLElement {
+  return ((component as any)[NG_HOST_SYMBOL] as LElementNode).native as any;


Similar to getRootContext, this may benefit from ngDevMode asserts to verify the elementNode is present.

JoostK · 2018-02-05T20:38:45Z

packages/core/src/render3/component.ts

+ * Retrieves the rendered text for a given component.
+ *
+ * This function retrieves the host element of a component and
+ * than returns the `textContent` for that element. This implies


and then returns

JoostK · 2018-02-05T20:43:41Z

packages/core/testing/src/render3.ts

+* adds HTML to the `body` element of the `document` and subsequently tears it down.
+*
+* This function is intended to be used with `async await` and `Promise`s. If the wrapped
+* function returns a promise (or is `async`) than the teardown is delayed until that `Promise`


a promise (or is async) then the teardown

JoostK · 2018-02-05T20:43:58Z

packages/core/testing/src/render3.ts

+* function returns a promise (or is `async`) than the teardown is delayed until that `Promise`
+* is resolved.
+*
+* On `node` this function detects if `document` is preset and if not it will create one by


detects if document is present and if not

chuckjaz · 2018-02-06T18:17:18Z

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

@@ -6,10 +6,10 @@
 * found in the LICENSE file at https://angular.io/license


Consider breaking this file into separate specs following the current nested defines, e.g. elements_spec.ts, component_and_directives_spec.ts, etc.

That is my plan, but I want to land this before I will break it up

ngbot · 2018-02-07T20:30:54Z

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

JoostK · 2018-02-08T08:01:22Z

packages/core/src/render3/component.ts

+ */
+export function markDirty<T>(component: T) {
+  const rootContext = getRootContext(component);
+  if (!rootContext.cleanResolve) {


Come to think of it, we only use rootContext.cleanResolve as a marker to indicate if the context is marked dirty, the function itself is not called as such as it is captured by the scheduled action closure below. We might as well choose not to expose the resolve function in RootContext but only have a dirty flag.

Good point, refactored

ngbot · 2018-02-08T17:24:10Z

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

kara · 2018-02-13T04:42:00Z

packages/core/src/render3/component.ts

@@ -157,9 +171,15 @@ export const NULL_INJECTOR: Injector = {


 /**
- * Bootstraps a Component into an existing host element and returns an instance
+ * Bootstraps a Component into an existing host element and return an instance


Intentional change? I think returns is actually clearer

kara · 2018-02-13T19:04:29Z

packages/core/src/render3/component.ts

+  /**
+   * A function which is used to schedule change detection work in the future.
+   *
+   * When marking components as dirty it is necessary to schedule the work of


Missing comma?

When marking components as dirty, it is necessary

kara · 2018-02-13T19:24:16Z

packages/core/src/render3/component.ts

+ * of this function will create a separate tree of components, injectors and
+ * change detection cycles and lifetimes. To dynamically insert a new component
+ * into an existing tree such that it shares the same injection, change detection
+ * and object lifetime use {@link ViewContainer#createComponent}.


Missing comma?

object lifetime, use ...

kara · 2018-02-13T19:45:39Z

packages/core/src/render3/component.ts

-    isDirty = true;
-    scheduler(() => detectChanges(component));
+  const lElementNode = (component as any)[NG_HOST_SYMBOL] as LElementNode;
+  ngDevMode && assertNotNull(lElementNode, 'Not a component!');


Replace lines 274-275 with _getComponentHostElementNode?

kara · 2018-02-13T19:47:00Z

packages/core/src/render3/component.ts

+ * This function returns a `Promise` which is resolved when the component's
+ * change detection is executed. This is determined by finding the scheduler
+ * associated with the `component`'s render tree and waiting until the scheduler
+ * flushes. If nothing is scheduled the function returns a resolved promise.


Missing comma? If nothing is scheduled, the function

kara · 2018-02-13T19:53:35Z

packages/core/src/render3/interfaces/view.ts

+  cleanPromise: Promise<null>;
+
+  /**
+   * `clearPromise` resolution function when the change detection finishes.


Name outdated in this comment? Re names, what do you think about markAsClean?

removed per #22018 (comment)

kara · 2018-02-13T19:56:05Z

packages/core/src/render3/interfaces/view.ts

+   *
+   * This promise is overwritten every time a first call to {@link markDirty} is invoked.
+   */
+  cleanPromise: Promise<null>;


Nit: I read this the first time as "a promise that was clean" rather than "a promise that is resolved when components are clean". How about onClean or componentsClean?

removed per #22018 (comment)

kara

LGTM

trotyl · 2018-02-14T06:14:29Z

Sorry for an off-topic question, is there any plan to land the scheduler feature in markForCheck to current view engine (hope there's a codename for it as well), rather than relying on Zone-dependent onMicrotaskEmpty? That could help a lot for using Angular without loading Zone.js.

From the code seems that part has little coupling to ivy itself.

mhevery · 2018-02-14T17:22:20Z

Sorry for an off-topic question, is there any plan to land the scheduler feature in markForCheck to current view engine (hope there's a codename for it as well), rather than relying on Zone-dependent onMicrotaskEmpty? That could help a lot for using Angular without loading Zone.js.

Yes, that is our plan, but we want to make sure that it is working well with Ivy before we back port it into ViewEngine

ngbot · 2018-02-15T00:17:28Z

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

mhevery · 2018-02-17T01:31:02Z

http://test/OCL:186069202:BASE:186069233:1518831050032:db1b279c

) PR Close #22018

PR Close #22018

PR Close angular#22018

…ular#22018) PR Close angular#22018

PR Close angular#22018

…ular#22018) PR Close angular#22018

PR Close angular#22018

…ular#22018) PR Close angular#22018

PR Close angular#22018

angular-automatic-lock-bot · 2019-09-14T18:22:00Z

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 Feb 5, 2018

trotyl reviewed Feb 5, 2018

View reviewed changes

JoostK reviewed Feb 5, 2018

View reviewed changes

chuckjaz reviewed Feb 6, 2018

View reviewed changes

mhevery force-pushed the sprint3 branch 2 times, most recently from 62c5469 to 2fcfa61 Compare February 8, 2018 06:55

JoostK reviewed Feb 8, 2018

View reviewed changes

kara reviewed Feb 13, 2018

View reviewed changes

mhevery force-pushed the sprint3 branch from 2fcfa61 to 265fba5 Compare February 13, 2018 23:12

kara approved these changes Feb 13, 2018

View reviewed changes

mhevery force-pushed the sprint3 branch from 265fba5 to 4947f59 Compare February 13, 2018 23:33

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

mhevery force-pushed the sprint3 branch 4 times, most recently from 9e7252b to 479f694 Compare February 14, 2018 05:55

mhevery added this to To do in Framework via automation Feb 14, 2018

mhevery added the area: core Issues related to the framework runtime label Feb 14, 2018

mhevery moved this from To do to In progress in Framework Feb 14, 2018

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

mhevery force-pushed the sprint3 branch from 479f694 to cb52186 Compare February 14, 2018 19:27

mhevery force-pushed the sprint3 branch 2 times, most recently from ae11db5 to fb929d3 Compare February 15, 2018 00:40

mhevery force-pushed the sprint3 branch 4 times, most recently from b521f8b to ffe68af Compare February 17, 2018 01:25

mhevery force-pushed the sprint3 branch from ffe68af to 4f81013 Compare February 17, 2018 01:33

mhevery added 3 commits February 16, 2018 20:33

docs: update BAZEL.md crosstool error instructions

04e4a06

test(ivy): move compiler canonical specs into a single directory

5afba7f

test(ivy): Add small_app spec for sprint #3

53c993e

mhevery force-pushed the sprint3 branch from 4f81013 to 53c993e Compare February 17, 2018 04:33

vicb closed this in 2654357 Feb 18, 2018

vicb pushed a commit that referenced this pull request Feb 18, 2018

test(ivy): move compiler canonical specs into a single directory (#22018

a63b764

) PR Close #22018

Framework automation moved this from in review to Done / Merged Feb 18, 2018

vicb pushed a commit that referenced this pull request Feb 18, 2018

test(ivy): Add small_app spec for sprint #3 (#22018)

ac2b04a

PR Close #22018

jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

docs: update BAZEL.md crosstool error instructions (angular#22018)

3c10b69

PR Close angular#22018

jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

test(ivy): move compiler canonical specs into a single directory (ang…

0ae56a7

…ular#22018) PR Close angular#22018

jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

test(ivy): Add small_app spec for sprint angular#3 (angular#22018)

cee13d3

PR Close angular#22018

smdunn pushed a commit to smdunn/angular that referenced this pull request Feb 28, 2018

docs: update BAZEL.md crosstool error instructions (angular#22018)

9ec74cd

PR Close angular#22018

smdunn pushed a commit to smdunn/angular that referenced this pull request Feb 28, 2018

test(ivy): move compiler canonical specs into a single directory (ang…

406ab1b

…ular#22018) PR Close angular#22018

smdunn pushed a commit to smdunn/angular that referenced this pull request Feb 28, 2018

test(ivy): Add small_app spec for sprint angular#3 (angular#22018)

6f320e4

PR Close angular#22018

trotyl mentioned this pull request Mar 5, 2018

Concept of Angular (ngZone + ChangeDetection) better than concept React, Vue (Virtual DOM)? #22587

Closed

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

docs: update BAZEL.md crosstool error instructions (angular#22018)

c2c644c

PR Close angular#22018

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

test(ivy): move compiler canonical specs into a single directory (ang…

81b0e9b

…ular#22018) PR Close angular#22018

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

test(ivy): Add small_app spec for sprint angular#3 (angular#22018)

a73666c

PR Close angular#22018

kara added the comp: ivy label Feb 25, 2019

ngbot bot added this to the needsTriage milestone Feb 25, 2019

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

		@@ -6,10 +6,10 @@
		* found in the LICENSE file at https://angular.io/license

Sprint3 #22018

Sprint3 #22018

Conversation

mhevery commented Feb 5, 2018 • edited

PR Checklist

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 Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngbot bot commented Feb 8, 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

kara left a comment

Choose a reason for hiding this comment

trotyl commented Feb 14, 2018

mhevery commented Feb 14, 2018

ngbot bot commented Feb 15, 2018

mhevery commented Feb 17, 2018

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

mhevery commented Feb 5, 2018 •

edited