fix(ivy): support separate creation mode and update mode execution

[kara]

Currently, the update block (i.e. binding refresh) always runs with the creation block (i.e. creating elements / directives) because of the way template functions are constructed. While this is the correct behavior for most templates, this limitation breaks dynamically created views because structural directives like ngFor expect that the creation block runs separately from the update block, as it does in render2.

This PR separates the execution of the creation block and update block, so they can run together or separately depending on what's appropriate. This is accomplished by replacing the boolean cm parameter in template functions with an enum called RenderFlags. Note that the order of the parameters has also been switched.

Before:

function Template(ctx: any, cm: boolean) {
  if (cm) {
    E(0, 'div');
    e();
  }
  p(0, 'title', b(ctx.title));
}

After (flags written out for clarity):

function Template(rf: RenderFlags, ctx: any) {
  if (rf & RenderFlags.Create) {
    E(0, 'div');
    e();
  }
  if (rf & RenderFlags.Update) {
    p(0, 'title', b(ctx.title));
  }
}

After (actual output):

function Template(rf: RenderFlags, ctx: any) {
  if (rf & 1) {
    E(0, 'div');
    e();
  }
  if (rf & 2) {
    p(0, 'title', b(ctx.title));
  }
}

Notes:

• For now, normal views and inline embedded views default to running the creation block and the update block together, while dynamically created views default to running them separately.
  • For dynamically created views, this means that when createEmbeddedView is called, only the creation block is run. The update block isn't executed until renderEmbeddedTemplate is called by refreshDynamicChildren at the end of the parent view's execution.
• Local variables that were previously declared outside of the creation block are now declared outside of either block (to support references in later container templates).

Before:

if (cm) {
  E(0, 'input', null, ['foo', '']);
  e();
  T(2);
}
const tmp = ld(1) as any;
t(2, b(tmp.value));

After:

if (rf & RenderFlags.Create) {
  E(0, 'input', null, ['foo', '']);
  e();
  T(2);
}
const tmp = ld(1) as any;
if (rf & RenderFlags.Update) {
  t(2, b(tmp.value));
}

• Previously, we would check if it was creation mode before setting bindingStartIndex. This doesn't work anymore because bindings won't always run with creation mode, so the bindingStartIndex might not be set. Now we have an explicit check to bindingStartIndex.
• Normally, text nodes with bindings are not created until their bound value becomes available in update mode (for performance reasons). So if there is a bound text node that is a root node of a dynamically created view, ViewContainerRef will try to insert it into the view before the text node has actually been created (update mode has not yet run at this point). In these cases, we have to create the text node at insertion, then update the value later.

[mary-poppins]

You can preview c3e1c1d at https://pr23292-c3e1c1d.ngbuilds.io/.

[vicb]

Would it make sense to have a ParenthesizedExpression extending (and wrapping an) Expression instead ?

[kara]

Can you clarify a bit what you're proposing? The change here is to make parentheses optional because currently they are always added to every Expression (which includes BinaryOperatorExpr). This means that without this change, we'd get if ((rf & 1)) instead of if (rf & 1). I've added a parameter so you can remove the parentheses if you know the expression you're generating doesn't need them.

[vicb]

as LViewNode should no more be required (*2)

[vicb]

May be explain what is the context here and we have both Create and Update set vs when we have Create and Update distinct ?

[vicb]

root -> first node ?

[kara]

In this case, it's not necessarily the first node. It's a node directly on the root component. I think in this case "root" is clearer because it's how we're referring to the same nodes elsewhere.

[vicb]

maybe add that this is a bound text node add xlink the place where there are usually created.

[vicb]

• not sure about theimplementation for ast.parens ?
• not sure about why we move var declaration to const outside of the Update and Create blocks (may be needs an ex of where it would not work ?),
• a few nits

Overall great changes !

[mhevery]

Why did we gain a new method here?

[kara]

We're not actually gaining any new code; I just refactored a duplicative part of textBinding, addRemoveView, and text into one shared function.

[kara]

presubmit

[mary-poppins]

You can preview 1afd4e1 at https://pr23292-1afd4e1.ngbuilds.io/.

[mary-poppins]

You can preview d9713d2 at https://pr23292-d9713d2.ngbuilds.io/.

[mary-poppins]

You can preview 45d5c5f at https://pr23292-45d5c5f.ngbuilds.io/.

[vicb]

currentView.bindingIndex should be null when entering the fn right ?
Add an assert

[vicb]

nit: === null

[vicb]

What about either have a number|null type or using -1 ? (and get ride of that ugly !)

[kara]

Yeah, I originally had it as number|null, but then you have to use the ! everywhere that you increment the binding index (which ends up being more ! overall)

[mary-poppins]

You can preview 7d30e19 at https://pr23292-7d30e19.ngbuilds.io/.

[mary-poppins]

You can preview 63920c5 at https://pr23292-63920c5.ngbuilds.io/.

[mary-poppins]

You can preview 92cabc7 at https://pr23292-92cabc7.ngbuilds.io/.

[angular-automatic-lock-bot]

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.}