Sprint3 by mhevery · Pull Request #22018 · angular/angular

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

matadatad.json? Typo?

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

Broken selector?

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

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

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

until all components are marked as dirty and then perform

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

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

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

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

and then returns

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

a promise (or is async) then the teardown

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

detects if document is present and if not

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

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

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

Intentional change? I think returns is actually clearer

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

Missing comma?

When marking components as dirty, it is necessary

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

Missing comma?

object lifetime, use ...

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

Replace lines 274-275 with _getComponentHostElementNode?

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

Missing comma? If nothing is scheduled, the function

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

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

removed per #22018 (comment)

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

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

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 the area: core Issues related to the framework runtime label 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 5 times, most recently from 467a0fb to 9094e4e Compare February 16, 2018 00:11

mhevery force-pushed the sprint3 branch 2 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

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

trotyl mentioned this pull request Mar 5, 2018

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

Closed

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

Conversation

mhevery commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngbot Bot commented Feb 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngbot Bot commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mhevery commented Feb 5, 2018 •

edited

Loading