ractivejs / ractive

Next-generation DOM manipulation
http://ractive.js.org
MIT License
5.94k stars 396 forks source link

Awaiting set with transitions from an observer resolves before transition ends #3295

Closed marcalexiei closed 5 years ago

marcalexiei commented 5 years ago

Description:

Hi there!

Looks like awaiting a set from an observer doesn’t actually waits for all relative transitions to be completed.

Background on the issue

You can skip reading this section if you’re not interested on the why.

We were working on a reimplementation of our routing logic, which involves a single component PageComponent to be replaced by the current active page component, based on external factors (URL patterns matched via Crossroads.js).

Since pages may contain intro/outro transitions, our idea was to set up an observer on pageName change, set a flag to remove the page, and awaiting its completition before unsetting the flag and inserting the new PageComponent.

Since this is part of a very complex setup, we’ve simplified the issue as much as possible.

The issue

ractive-issue

Versions affected:

Tested on

Platforms affected:

All, tested on Chrome and Safari

Reproduction:

jsfiddle

const Application = Ractive.extend({
  target: '#application',

  template: `
  <div>Counter is {{counter}}</div> <!-- If we remove this line everything works correctly-->

  {{#if renderContent}}
    <h1 fade-in-out="{ duration: 2000 }">Lorem ipsum</h1>
  {{/if}}
  `,

  data() {
    return {
      counter: 0,

      renderContent: true,
    };
  },

  observe: {
    counter: {
      async handler() {
        await this.set({ renderContent: false });

        // content should NOT be visible here.

        await this.set({ renderContent: true });
      },
    },
  },
});

const app = new Application();

setInterval(() => {
  app.add('counter');
}, 2500);
evs-chris commented 5 years ago

I think ractive is behaving correctly, at least from the fiddle. Does the real scenario involve and observer causing mutations? If so, I think you'll get the desired behavior by using a deferred observer: https://jsfiddle.net/jqeugwfc/

If not, lemme know and I'll dig in more.

marcalexiei commented 5 years ago

I'll add more background: In our app counter is an object which contains URL information and {{#if}} content is a component.

Does the real scenario involve an observer causing mutations?

Yes, however using deferred option the new component gets rendered in the page but route doesn't have updated information and this is leading to errors / unexpected results.

Desired behavior will be:

  1. route change.
  2. observer callback is fired.
  3. old component is removed (first await)
  4. we change page component inside components property.
  5. render new page component (second await)

If not clear on monday I'll provide a fiddle with a more accurate example.

evs-chris commented 5 years ago

It's looking like the way ractive is designed won't really support this particular scenario as described. The runloop promises will nest, but because only one loop can own fragment updates at a time and the fragment is already registered when the observer fires (because the counter bubbled an update up and caused it to register), the second nested loop can't pick up the change to the h1. That moves the removal out of the promise returned by the second loop, so it resolves immediately, allowing the addition to immediately fire within the observer.

What about pushing the observer out of the main loop entirely via the microtask queue by something like await 1 or setTimeout? Does that work in your scenario?

marcalexiei commented 5 years ago

While trying to find a workaround we found that a similar problem occurs with deferred observers dealing with node element with transitions:

  1. Open this fiddle
  2. Open the inspector (we are using Chrome)
  3. Run the snippet
  4. JS execution should stop inside observer which has defer: true

Expected result: "Lorem ipsum" is not displayed in the page

Actual result "Lorem ipsum is displayed"

If we remove the transition everything works fine.

const Application = Ractive.extend({
  target: '#application',

  template: `
  {{#if isVisible}}
    <h1 fade-in-out="{ duration: 50 }">Lorem ipsum</h1>
  {{/if}}
  `,

  data() {
    return {
      isVisible: true,
    };
  },

  observe: {
    isVisible: {
      async handler() {
        debugger
      },
      defer: true,
      init: false,
    },
  },
});

const app = new Application();

setTimeout(() => {
  app.toggle('isVisible');
}, 2500);
evs-chris commented 5 years ago

That's actually expected as well, because deferred observers don't wait on transitions. They only wait on changes to the dom. The flow is call dirty observers -> apply fragment changes -> call deferred observers -> repeat until no changes are queued -> start transitions -> resolve promise when transitions complete. If there are no transitions on an element that is flagged for removal, it is removed immediately when the start transitions is called. It can't remove them at fragment change application because there may be transitions as ancestors or descendants that are transitioning that need them to stick around.

Would it help to have access to the current operation's promise in the observer? If that's the case, we could probably add a property onto Ractive that would give you access to that.

marcalexiei commented 5 years ago

It will probably help to find a feasible solution with our problem.

we could probably add a property onto Ractive that would give you access to that

With Ractive you mean the global Ractive object or the instance where the observer is attached?

evs-chris commented 5 years ago

It's on the Ractive constructor because there's only one runloop that covers all instances. I've pushed this to edge experimentally as Ractive.tick if you want to see how it works for you. I'm also not set on the name, but it seems to be close to what other libraries use for similar concepts.

marcalexiei commented 5 years ago

We have tried to use the tick property, however it still not working in our scenario so we have opted to an alternative solution which doesn't involve observers: we remove the component in router callback before setting the new route. So far by now it seems to working.

Anyway I think that Ractive.tick is a very useful addition to Ractive because will reduce setTimeout and await 1 hack usage.

Thank you for your effort!