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

[BUGFIX release] Ensure tag updates are buffered #18698

Merged
merged 2 commits into from
Jan 25, 2020

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Jan 24, 2020

This update ensures that various forms of tag updates are buffered, and
also adds tests for edge cases concerning combinations of computed
properties, lazy dependencies, and observers that triggered this
assertion.

Chris Garrett added 2 commits January 24, 2020 13:46
This update ensures that various forms of tag updates are buffered, and
also adds tests for edge cases concerning combinations of computed
properties, lazy dependencies, and observers that triggered this
assertion.
@pzuraq pzuraq merged commit deefc2e into release Jan 25, 2020
@pzuraq pzuraq deleted the bugfix/release/ensure-tag-updates-are-buffered branch January 25, 2020 18:05
@lifeart
Copy link
Contributor

lifeart commented Jan 29, 2020

I seen one more related issue.

Given: tracked calculations chain, dependent on dom node values.

Dom node setted as property value using {{ref}} modifier.

import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
export default class ProgressIndicatorComponent extends Component {
  @tracked
  progressContainer = null;
  @tracked
  progressItems = [];
  get maxAmount() {
    return  Math.floor(this.progressContainerWidth / 36) - 5;
  }
  get itemsLength() {
    return this.progressItems.length;
  }
  get completedItems() {
    return (this.progressItems||[]).filter(({completedInCurrentCycle})=>completedInCurrentCycle === true);
  }
  get currentItemInProgress() {
    return this.itemsLength - this.completedItems.length - 1;
  }
  get progressContainerWidth() {
    return this.progressContainer && this.progressContainer.offsetWidth || 0;
  }
  get shouldHideExtraItems() {
    return this.maxAmount < this.itemsLength;
  }
  get itemsToHideCount() {
    const completedToHide =
    this.completedItems.length - Math.floor(this.maxAmount / 2);
    return completedToHide >= 0 ? completedToHide : 0;
  }
  get hiddenUncompletedCount() {
    const amount = this.itemsLength - this.itemsToHideCount - this.maxAmount;
    return amount;
  }
  get negativeHiddenUncompletedCount() {
    return this.hiddenUncompletedCount < 0 ? this.hiddenUncompletedCount : 0;

  }
  get positiveHiddenUncompletedCount() {
    return this.hiddenUncompletedCount > 0 ? this.hiddenUncompletedCount : 0;

  }
  get hiddenCompletedCount() {
    return this.itemsToHideCount + this.negativeHiddenUncompletedCount;

  }
  get betweenPadding() {
    return this.shouldHideExtraItems
      ? this.progressContainerWidth - 36 * this.maxAmount - 5
      : 0;
  }
}
<div>
{{! template-lint-configure no-inline-styles false }}
<div class="mb-3">
  <nav id="progressContainer" {{ref this 'progressContainer'}} class="c-pagination c-pagination--tasks mb-3">
    {{#if @progressItems.length}}
      <ul
        data-test-items-list
        data-test-max-amount={{this.maxAmount}}
        class="clearfix items-list"
        style={{html-safe
          (concat
            "width: calc(36px*"
            @progressItems.length
            ");"
            (if
              this.shouldHideExtraItems
              (concat
                "right: calc(36px*(-"
                (sum this.itemsToHideCount this.negativeHiddenUncompletedCount)
                "))"
              )
            )
          )
        }}
      >
        {{#each (sort-by "order:desc" @progressItems) as |item index|}}
          <li
            data-test-progress-indicator-item
            data-test-progress-indicator-item-number={{item.order}}
            class="tasks-pagination-item

              {{if item.nextAttempt " tasks-pagination-item--next-attempt"}}"
            style={{html-safe
              (concat
                "width: calc((100% - 36px*"
                index
                ") + "
                (if this.shouldHideExtraItems this.betweenPadding -5)
                "px);"
                (if
                  item.completedInCurrentCycle
                  (concat
                    "transform: translateX(calc(100% - 36px*"
                    (subtract @progressItems.length index)
                    "));"
                  )
                )
              )
            }}
          >
            {{#let
              (not
                (or
                  item.completedInCurrentCycle
                  (eq index this.currentItemInProgress)
                )
              ) as |shadeItem|
            }}
              <span
                data-test-shaded-progress-circle-element={{shadeItem}}
                class="inline-block {{if shadeItem "opacity-50"}}"
              >
              </span>
            {{/let}}
          </li>
        {{/each}}
      </ul>
    {{else}}
    no tasks
    {{/if}}
  </nav>
  {{#if this.shouldHideExtraItems}}
    <div class="flex justify-between hidden-counter">
      <span data-test-hidden-uncompleted>
        {{log 'data-test-hidden-uncompleted' 'positiveHiddenUncompletedCount' this.positiveHiddenUncompletedCount}}
        {{if
          this.positiveHiddenUncompletedCount
          (concat "+" this.positiveHiddenUncompletedCount)
        }}
      </span>
      <span data-test-hidden-completed>
         {{log 'data-test-hidden-completed' 'hiddenCompletedCount' this.hiddenCompletedCount}}

        {{if this.hiddenCompletedCount (concat "+" this.hiddenCompletedCount)}}
      </span>
    </div>
  {{/if}}
</div>
</div>

image

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 29, 2020

I believe that since modifiers run after render, this is expected behavior, and the autotracking transaction is operating correctly in this case. The tracked property has already been used once during render, and now it is being updated during the same render, which is invalid.

@lifeart
Copy link
Contributor

lifeart commented Jan 29, 2020

@pzuraq as I understand there is no 'valid' way to get DOM access at all during initial render?

@lifeart
Copy link
Contributor

lifeart commented Jan 29, 2020

@pzuraq one more question, how modifiers can correctly set property value on component? (to avoid such transaction error) sheduleOnce('afterRender') `?

// https://github.com/lifeart/ember-ref-modifier

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 29, 2020

There is, within modifiers. You can reverse your code flow such that the modifier receives a percentage, and internally figures out the width it should display on the element.

It's not valid to have state backflow from a modifier though, which is what is happening here. This was already scheduling two rendering passes as is, this just prevents that from occurring.

@lifeart
Copy link
Contributor

lifeart commented Jan 29, 2020

@pzuraq if modifiers run after render, why this is same transaction?

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 29, 2020

The transaction is meant to prevent backflow. Modifiers run last, and if they mutate any state, then we have to completely start over, because everything else is already done.

For the other question, you can do run('next') to manually schedule another runloop, but this is really not advisable in the long term, since it could result in numerous runloops and potentially infinite rerendering issues. Currently this flushes synchronously, but I definitely imagine that in the future we will have an effort to make each individual render callback be a single render pass so that it's more efficient, and so relying on run('next') would result in multiple actual rerenders to settle state (and thus, glitches in your app).

This is why we built the assertion, to help users find and prevent these types of state flow issues.

@lifeart
Copy link
Contributor

lifeart commented Jan 29, 2020

Thank you for clarification!

As I understand, DOM related tracked behavour must be implemented as callback?

<div {{get-inner-width this.onInnerWidth}} />

and this callback can't mutate any value in current transaction, so, we need to call this function in next runloop.

@pzuraq
Copy link
Contributor Author

pzuraq commented Jan 29, 2020

Ah, I see, your UI requires measuring before it can render at all. In this case then yes, you would need schedule updates for the next runloop and rerender.

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

Successfully merging this pull request may close these issues.

3 participants