refactor(ivy): rename objectLiteral to pureFn to prep for pipes #22214

kara · 2018-02-14T03:00:53Z

Renames objectLiteral instructions to pureFunction because pipes will soon share the code. Also adds a test to the canonical spec for array literals that will not change.

mary-poppins · 2018-02-14T03:10:26Z

You can preview 4f0f8cf at https://pr22214-4f0f8cf.ngbuilds.io/.

vicb · 2018-02-14T17:15:28Z

packages/core/src/render3/pure_function.ts

The header comment l12 as well as Function that returns an updated instance of the object/array are no more accurate.
This is intended to be used with any pure function.

Good catch; forgot about those comments

vicb · 2018-02-14T17:18:08Z

packages/core/src/render3/pure_function.ts

wondering if argX or argValX makes more sense than latestValue now (thinking out loud)

Hmm out of the 3, I think I like latestValue best. argX doesn't capture how we are getting the latest value of the binding out of data[]

vicb · 2018-02-14T17:24:27Z

packages/core/test/render3/compiler_canonical_spec.ts

why do we need 2 p ?
{{ names[0] }}{{ names[1] }} could remove some noise ?

Eh, no real reason, just for organization. I can remove if you think it creates noise.

vicb · 2018-02-14T17:25:18Z

packages/core/test/render3/compiler_canonical_spec.ts

nit: with no changes -> of constants (it could be an exp that doesn't change)

vicb · 2018-02-14T17:29:47Z

packages/core/test/render3/compiler_canonical_spec.ts

I would use bind0 for the sake of consistency and to save 1 byte.

c?$e0_arr$:$r3$.ɵNC; $r3$.ɵb0($e0_arr$);

If we send the array to bind, then we are calling isDifferent every time in addition to bind, as well as storing the value unnecessarily in data[]. I think it makes more sense for it to be treated the same way a creation-time binding is treated.

// template function $r3$.ɵp(0, 'names', $r3$.ɵb0($e0_arr$); // instructions.ts export function bind0(value: any): any|NO_CHANGE { return creationMode ? value : NO_CHANGE; }

Read the above as b() not b0(). Now your comment makes sense!

vicb · 2018-02-14T17:33:51Z

packages/core/test/render3/pure_function_spec.ts

I think that having describe('array literals', ...) and describe('object literals', ...) does not really make sense any more but it is probably ok and related to our tests not being real unit tests ?
WDYT @mhevery

I think the names should stay, given that those are the actual template features we are testing in integration tests. pureFunctions wouldn't actually describe the scenario adequately.

I am fine either way.

mary-poppins · 2018-02-14T18:02:13Z

You can preview 6696fc5 at https://pr22214-6696fc5.ngbuilds.io/.

mhevery · 2018-02-14T17:59:33Z

packages/core/src/render3/pure_function.ts

we have also discussed adding optional applyThis: any to the arg list.

Also we need pureFunction0. Imagine this binding {{ ['foo'].length + 1 }}. In this case you need to prevent new object re-creation so you need pureFunction0.

function create0() { return ['foo']; } text(0, pureFunction(create0).length + 1);

mhevery · 2018-02-14T18:02:59Z

packages/core/src/render3/pure_function.ts

applyThis ? pureFn.call(applyThis, latestValue): pureFn(latestValue)

see: https://jsperf.com/function-calls-direct-vs-apply-vs-call-vs-bind/6

mhevery · 2018-02-14T18:12:58Z

packages/core/test/render3/compiler_canonical_spec.ts

Add a use case for {{ ['foo'].length + 1 }}

mhevery · 2018-02-14T18:13:28Z

packages/core/test/render3/pure_function_spec.ts

I am fine either way.

mhevery · 2018-02-14T22:46:59Z

packages/core/src/render3/instructions.ts

nit: In lexer/parsere terminology peek does not change state and consume does. Your peekAndIncrementBinding is confusing since it seems to be peek (no side
effect) and than it has side-effect. Could you rename to consumeBinding. Also can you give it explicit return type of any

mhevery · 2018-02-14T22:48:03Z

packages/core/src/render3/pure_function.ts

We discussed that returning NO_CHANGE is never correct. What is your plan here?

NO_CHANGE :)

mhevery · 2018-02-15T00:56:41Z

packages/core/src/render3/pure_function.ts

bind(pureFn(exp)) could return NO_CHANGE so it is still not quite right.

I think this is what we are looking for:

return bind(exp) !== NO_CHANGE ? updateBinding(pureFn(exp)) : consumeBinding();

maybe this may be more readable:

return consumeBinding() === exp ? consumeBinding() : updateBinding(pureFn(exp));

PS: assuming we have updateBinding function.

I assumed the factory function would always return a new instance of array/object, but @mhevery points out that it could return a constant if used in pipes (for which we don't yet have tests).

mary-poppins · 2018-02-15T01:04:27Z

You can preview b775e05 at https://pr22214-b775e05.ngbuilds.io/.

mary-poppins · 2018-02-15T02:19:25Z

You can preview 510b76d at https://pr22214-510b76d.ngbuilds.io/.

mary-poppins · 2018-02-15T02:21:41Z

You can preview 70179a8 at https://pr22214-70179a8.ngbuilds.io/.

mhevery · 2018-02-15T03:27:02Z

packages/core/test/render3/integration_spec.ts

This is wrong. You can't use bind0 since a one time binding has to short circuit the evaluation of the expression. In this case value. So the original code was correct.

mhevery · 2018-02-15T03:28:31Z

packages/core/src/render3/instructions.ts

I don't think there is a use case for this. If you want one time binding than we have to make sure that the value is never evaluated, otherwise this is pointless. The idea of one time binding is that the expression (not checking if it has changed) is expensive and hence needs to be short circuited.

mhevery · 2018-02-15T03:35:16Z

packages/core/src/render3/pure_function.ts

This is good, but I am wondering if bind is the right method since it is more complicated than it needs to be. a simpler method may be faster.
return isDifferent(consumeBinding(), exp) ? updateBinding(pureFn(exp)) : consumeBinding();

We could do that, but we'd also have to add some logic on top to make sure the binding value is saved for the exp alone (so that when we call consumeBinding, it already exists). e.g.

if (getCreationMode() || isDifferent(consumeBinding(), exp)) { updateBinding(exp); updateBinding(pureFn(exp)); } else { consumeBinding(); }

This essentially does the same thing as bind, but in two parts (updateBinding + isDifferent), so I think it's better to use bind.

mary-poppins · 2018-02-15T20:56:03Z

You can preview 6377d8b at https://pr22214-6377d8b.ngbuilds.io/.

mary-poppins · 2018-02-16T03:06:41Z

You can preview fb931de at https://pr22214-fb931de.ngbuilds.io/.

mhevery · 2018-02-16T03:55:40Z

packages/core/src/render3/pure_function.ts

+    bindingUpdated(exps[i]) && (different = true);
+  }
+
+  return different ? checkAndUpdateBinding(pureFn(exps)) : consumeBinding();


Sorry this needs to be pureFn.apply(null, exps). I thought we were arguing about signature of pureFunctionV which will get invoked more often. But for pureFn we should be consistent.

ngbot · 2018-02-16T05:00:50Z

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: build" is pending
     status "ci/circleci: lint" is pending
     status "continuous-integration/travis-ci/pr" 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.

kara · 2018-02-16T05:03:18Z

presubmit

mary-poppins · 2018-02-16T05:05:24Z

You can preview 4b41c96 at https://pr22214-4b41c96.ngbuilds.io/.

vicb · 2018-02-16T19:35:56Z

packages/core/src/render3/instructions.ts

+  creationMode && initBindings();
+
+  if (creationMode || isDifferent(data[bindingIndex], value)) {
+    data[bindingIndex++] = value;


only call initBindings() here;

assert that value is no NC (please also add the same assert in consumeBinding above for the returned value

mary-poppins · 2018-02-16T21:50:14Z

You can preview 778149f at https://pr22214-778149f.ngbuilds.io/.

mary-poppins · 2018-02-16T22:06:06Z

You can preview fc8bc18 at https://pr22214-fc8bc18.ngbuilds.io/.

PR Close #22214

angular-automatic-lock-bot · 2019-09-14T04:22:48Z

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

kara added action: review The PR is still awaiting reviews from at least one requested reviewer comp: ivy labels Feb 14, 2018

kara requested a review from mhevery February 14, 2018 03:01

vicb reviewed Feb 14, 2018

View reviewed changes

mhevery suggested changes Feb 14, 2018

View reviewed changes

mhevery reviewed Feb 14, 2018

View reviewed changes

mhevery reviewed Feb 15, 2018

View reviewed changes

kara added 10 commits February 15, 2018 18:45

refactor(ivy): rename objectLiteral to pureFn to prep for pipes

173957c

fixup! refactor(ivy): rename objectLiteral to pureFn to prep for pipes

666c4b4

fixup! refactor(ivy): rename objectLiteral to pureFn to prep for pipes

0ce247f

feat(ivy): add pureFunction0 instruction

92a34b1

fixup! feat(ivy): add pureFunction0 instruction

a471885

fixup! feat(ivy): add pureFunction0 instruction

35d56b6

fixup! feat(ivy): add pureFunction0 instruction

01aa8b2

fixup! feat(ivy): add pureFunction0 instruction

7a754f9

fixup! feat(ivy): add pureFunction0 instruction

3b543f8

fixup! feat(ivy): add pureFunction0 instruction

fb931de

kara force-pushed the pureFunction branch from 6377d8b to fb931de Compare February 16, 2018 02:58

mhevery approved these changes Feb 16, 2018

View reviewed changes

fixup! feat(ivy): add pureFunction0 instruction

4b41c96

mhevery approved these changes Feb 16, 2018

View reviewed changes

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

kara added the target: major This PR is targeted for the next major release label Feb 16, 2018

vicb reviewed Feb 16, 2018

View reviewed changes

fixup! feat(ivy): add pureFunction0 instruction

778149f

vicb approved these changes Feb 16, 2018

View reviewed changes

kara mentioned this pull request Feb 16, 2018

feat(ivy): implement pipes #22254

Closed

fixup! feat(ivy): add pureFunction0 instruction

fc8bc18

vicb closed this in a73d530 Feb 17, 2018

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

feat(ivy): add pureFunction0 instruction (#22214)

f693be3

PR Close #22214

kara deleted the pureFunction branch October 13, 2018 01:09

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

refactor(ivy): rename objectLiteral to pureFn to prep for pipes #22214

refactor(ivy): rename objectLiteral to pureFn to prep for pipes #22214

Uh oh!

Conversation

kara commented Feb 14, 2018

Uh oh!

mary-poppins commented Feb 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Feb 14, 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!

mary-poppins commented Feb 15, 2018

Uh oh!

mary-poppins commented Feb 15, 2018

Uh oh!

mary-poppins commented Feb 15, 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!

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

Uh oh!

vicb Feb 14, 2018 •

edited

Loading

vicb Feb 14, 2018 •

edited

Loading

kara Feb 14, 2018 •

edited

Loading

kara Feb 14, 2018 •

edited

Loading

kara Feb 15, 2018 •

edited

Loading