Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] {{each}} item binding with square brackets doesn't update, when right side is an aliased attribute #582

Open
ericyhwang opened this issue Apr 29, 2020 · 2 comments

Comments

@ericyhwang
Copy link
Contributor

ericyhwang commented Apr 29, 2020

The title is long and complicated. I have a repro of the issue using Derby's component tests:
ee2e1b0

Also putting the example here:

<Body:>
  <view is="greeting-list" greetingIds="{{_page.greetingIds}}"/>

<greeting-list:>
  {{each @greetingIds as #id, #i}}
    <div>{{_page.greetings[#id]}}</div>
  {{/each}}

Doing a model.set('_page.greetingIds.0', 'foo'), I'd expect the binding to update, but it doesn't.

The binding does update properly under these other scenarios:

  1. Setting the entire array: model.set('_page.greetingIds', ['foo'])
  2. Changing the template to use the array index: {{ _page.greetings[@greetingIds[#i]] }}
  3. Changing the template to not use an attribute: {{each _page.greetingIds as #id, #i}}
@ericyhwang ericyhwang changed the title Bug - {{each}} item binding with square brackets doesn't update when right side is an aliased attribute [Bug] {{each}} item binding with square brackets doesn't update, when right side is an aliased attribute Apr 29, 2020
@ericyhwang
Copy link
Contributor Author

ericyhwang commented Apr 29, 2020

Based on the fact that {{each _page.greetingIds}} does work, I've debugged it down to a difference between PathExpression#dependencies (works fine) and AttributePathExpression#dependencies (doesn't work).

In the screenshot, the left side is the working PathExpression, and the right side is the not-working AttributePathExpression.

Screen Shot 2020-04-29 at 3 32 29 PM

Specifically, PathExpression's call to appendDependency results in a correct dep list, but AttributePathExpression's call to swapLastDependency does not.


As for a fix - Just to see what happens, since I suspected the early-return to be an issue, I tried changing the initial early-return check in swapLastDependency:

https://github.com/derbyjs/derby-templates/blob/6cf8f1ba344c9aec59e577945efe4d17416fcd10/lib/expressions.js#L748

Like so:

  function swapLastDependency(dependencies, expression, context) {
-   if (!expression.segments.length) {
+   if (!expression.segments.length && !expression.meta) {
      return dependencies;
    }

That did fix the issue, and all the other tests pass... but I have no idea what the intent behind swapLastDependency is, nor do I know what that early-return is intended to do. My "see what happens" change is almost certainly not the correct fix.

@nateps can you provide some context / advise?

@JerryI
Copy link

JerryI commented Oct 19, 2020

It was an old bug which made me kill myself at the previous job. Thank you a ton, @ericyhwang!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants