Refactor Modals to Glimmer components

[simonihmig]

Most components refactored to template-only.

[jelhan]

Started a review some time ago but haven't had the time to finish it yet. Only reviewed API docs for <BsModalSimple> so far. I guess it's only a copy and paste. But noticed that some parts are outdated. Let's take the chance and fix that.

[jelhan]

Is using {{if}} helper working well with animations? I thought @open argument is required for animations.

[jelhan]

Reviewed the rest as well. Mostly general questions not strictly related to this refactoring. Keep request changes as I think the fixes to docs from last review should be included for sure.

[jelhan]

I get that changes of the value of @open argument must trigger side effects. But I would prefer if changes to internal state does not trigger side effects as it makes reasoning about state and its flow much harder.

I get that this is only replacing an existing observer. And that we shouldn't overcomplicate the glimmer refactoring by trying to address all technical debts at once.

But maybe we can add a comment that this should be refactored? Just to make it clear that this isn't a pattern which should be used for new features?

[simonihmig]

How would you refactor that?

But I would prefer if changes to internal state does not trigger side effects as it makes reasoning about state and its flow much harder.

But in our case that needs to happen. When setting our inter isOpen to true, we need to create side effects that are not possible just by auto-updating our template, like changing the <bod> class, adding a transitionEnd listener etc.

[jelhan]

When setting our inter isOpen to true, we need to create side effects that are not possible just by auto-updating our template, like changing the <bod> class, adding a transitionEnd listener etc.

That shouldn't be caused by a side effect but explicitly executed at the same place as isOpen is changed.

How would you refactor that?

Let me try to illustrate with some pseudo code.

{{did-insert this.reactOnExternalVisibilityChanges}}
{{did-update this.reactOnExternalVisibilityChanges @open}}

export default ModalComponent extends Component {
  @tracked isOpen = this.args.open;

  @action
  show() {
    if (this.isOpen) {
      // Modal is already open. Nothing to do.
    }

    this.isOpen = true;

    // ...
  }

  @action
  hide() {
    if (!this.isOpen) {
      // Modal is already hidden. Nothing to do.
    }

    this.isOpen = false;

    // ...
  }

  @action
  close() {
    if (this.args.onHide?.() !== false) {
      // Before this method calls `hide` method implicitly by setting
      // `this.isOpen = false`. This is the classical observer style.
      // Instead we should call the `hide` method explicitly.
      this.hide();
    }
  }

  @action  
  reactOnExternalVisibilityChanges() {
    if (this.args.open) {
      this.show();
    else {
      this.hide();
    }
  }
}

Not sure if I missed something. The general idea is that internal isOpen state is changed only through show and hide methods. Internal and yielded actions call these methods directly. External state passed in as @open argument is observed using {{did-update}} helper. If it changes show or hide method is executed.

{{did-insert}} is used caused DOM must be inserted before show method can be executed. I would have preferred to use constructor instead. But not sure if DOM is guaranteed to exist if scheduling if for afterRender or next runloop. That's why I didn't touched that part.

[jelhan]

Why next instead of afterRender? Isn't afterRender more precise?

[simonihmig]

Yes, but somehow not working. Like it didn't see the rendering changes when they callback ran. 🤔

[jelhan]

It seems as if the callback itself also schedules something for afterRender: https://github.com/kaliber5/ember-bootstrap/blob/75f12adb7070fd0d931bc6f90d8941c5bb957425/addon/components/bs-modal.js#L433-L462 Maybe it's just not well specified what happens if scheduling something for the run loop you are currently?
🤔

schedule('afterRender', this, function() {
  schedule('afterRender', this, function() {
    // Same runloop or after render of next runloop? 😕
  });
});

So executing the callback in actions queue of next runloop seems to be a cleaner approach. Not sure though if maybe only the callback should be delayed for next runloop but not the transitionEnd? Like this:

schedule('afterRender', this, function() {
  let backdrop = this.backdropElement;
  assert('Backdrop element should be in DOM', backdrop);
  if (doAnimate) {
    transitionEnd(backdrop, this.backdropTransitionDuration).then(callback);
  } else {
    next(() => {
      callback();
    });
  }
});

An alternative would be to change the code in the callback so that the timing of the other callback wouldn't be affected. The other usage of handleBackdrop doesn't seem to schedule something for afterRender: https://github.com/kaliber5/ember-bootstrap/blob/75f12adb7070fd0d931bc6f90d8941c5bb957425/addon/components/bs-modal.js#L498-L504

[jelhan]

Is this correct for a template only component?

[simonihmig]

Idk. They are after all called template only Glimmer components, aren't they? Not that this matters much here, given this is a private component (only yielded), and extending is not supported anyways...

[simonihmig]

Thanks @jelhan for your super thorough review! Applied your suggestions.

The failing tests in beta and canary are due to buschtoens/ember-render-helpers#282. 🤔

[simonihmig]

Updated ember-render-helpers which makes tests pass on CI.

@jelhan applied your suggestion regarding using {{did-update ... @open}} above (still need this._isOpen though) in the last commit. I did not have time to fiddle with the schedule timings you asked about though...

[jelhan]

@simonihmig Thanks a lot for pushing this over the finished line. Looks great!