miguelcobain / ember-composability-tools

ember-composability-tools - Helpers for building a somewhat different kind of components.
MIT License
39 stars 18 forks source link

Advice for dealing with ansync setup in parent components? #57

Closed jagthedrummer closed 4 months ago

jagthedrummer commented 4 months ago

I'm curious if there's any general advice for how to deal with async methods when using this addon. The setup process of my parent component takes some time, and then the didInsertParent of the child component is called before the parent component is fully ready. Is there a standard way of synchronizing everything?

I think that covers the general question, but just in case more details would be helpful here are some more.

This is the general structure that I'm going for.

<BouncePlayer::Wavesurfer @bounce={{@bounce}} as |wavesurfer|>
  <wavesurfer.regions as |regions|>
    <regions.marker @start={{this.start}} @end={{this.end}}/>
  </wavesurfer.regions>
</BouncePlayer::Wavesurfer::Display>

In the Regions component the didInsertParent method looks like this:

  @action
  async didInsertParent(element) {
    this.regions = await RegionsPlugin.create({});
    await this.args.wavesurfer.registerPlugin(this.regions); 
  }

Sometimes this works great, but sometimes it fails with an error saying Uncaught TypeError: Cannot read properties of null (reading 'registerPlugin') due to the wavesurfer object that is passed in from the parent not being full initialized yet.

Is there a standard way of dealing with this sort of situation?

miguelcobain commented 4 months ago

@jagthedrummer this is an interesting point you bring up. ember-composability-tools could be promise aware.

But for your specific use case, it seems to me that neither RegionsPlugin.create({}) or registerPlugin() return a promise.

I'm looking at this example: https://wavesurfer.xyz/examples/?regions.js

Can you share the code for the BouncePlayer::Wavesurfer component (js and template) and the Regions component as well? Just the relevant parts.

jagthedrummer commented 4 months ago

Thanks for the response, @miguelcobain!

I think you're right about neither of those methods returning a promise. Do you think that's the root of the problem? Like, would you expect ember-composability-tools to do what I'm asking for if they did return a promise?

Here are the relevant bits (I hope).

bounce-player/wavesurfer.js

export default class BouncePlayerWavesurferDisplayComponent extends Component {

  componentsToYield = [
    { name: 'timeline', as: 'timeline', component: BouncePlayerWavesurferTimelineComponent },
    { name: 'regions', as: 'regions', component: BouncePlayerWavesurferRegionsComponent },
  ];

  @action
  mergeComponents(obj) {
    if (!this.mergedComponents) {
      this.mergedComponents = obj;
    } else {
      Object.assign(this.mergedComponents, obj);
    }
  }

  @action
  async didInsertParent(element) {
    this._element = element;
    await this._buildWavesurfer.perform();
    this.setWavesurferTime();
  }

  @action
  willDestroyParent(element) {
    this._destroyWavesurfer.perform()
  }

  _buildWavesurfer = task(async () => {
    this.wavesurfer = await WaveSurfer.create({
      container: this._element,
      // ... a bunch of other (probably) unrelated options
    })
  })

  _destroyWavesurfer = task(async () => {
    if(this.wavesurfer){
      this.wavesurfer.empty();
      this.wavesurfer.unAll();
      this.wavesurfer.destroy();
      this.wavesurfer = null;
    }
  })
}

bounce-player/wavesurfer.hbs (This is basically lifted straight from leaflet-map.hbs in ember-leaflet. The seshy-* helpers are also lifted directly from ember-leaflet with no changes.)

<Root
  @didInsertParent={{this.didInsertParent}}
  @willDestroyParent={{this.willDestroyParent}}
  {{did-update this.setWavesurferTime @currentTime}}
  ...attributes
  as |Node|
>
  {{#let (seshy-hash) as |components|}}
    {{#each this.componentsToYield as |c|}}
      {{seshy-assign-to
        components
        key=(seshy-or c.as c.name)
        value=(component (ensure-safe-component (seshy-or c.component c.name)) parent=this node=Node wavesurfer=this.wavesurfer)
        onChange=this.mergeComponents
      }}
    {{/each}}

    {{yield this.mergedComponents}}
  {{/let}}
</Root>

wavesurfer/regions.js

export default class BouncePlayerWavesurferRegionsComponent extends Component {
  @tracked regions = null;

  componentsToYield = [
    { name: 'marker', as: 'marker', component: BouncePlayerWavesurferMarkerComponent }
  ];

  @action
  mergeComponents(obj) {
    if (!this.mergedComponents) {
      this.mergedComponents = obj;
    } else {
      Object.assign(this.mergedComponents, obj);
    }
  }

  @action
  async didInsertParent(element) {
    this._element = element;
    await this._buildRegions.perform();
  }

  @action
  willDestroyParent(element) {
    this._destroyRegions.perform()
  }

  _buildRegions = task(async () => {
    this.regions = await RegionsPlugin.create({});
    await this.args.wavesurfer.registerPlugin(this.regions);
  })

  _destroyRegions = task(async () => {
    if(this.regions){
      this.regions.destroy();
      this.regions= null;
    }
  })
}

wavesurfer/regions.hbs

<@node
  @didInsertParent={{this.didInsertParent}}
  @willDestroyParent={{this.willDestroyParent}}
  @tagName="div"
  ...attributes
  as |Node|
>
  {{#let (seshy-hash) as |components|}}
    {{#each this.componentsToYield as |c|}}
      {{seshy-assign-to
        components
        key=(seshy-or c.as c.name)
        value=(component (ensure-safe-component (seshy-or c.component c.name)) parent=this node=Node regions=this.regions)
        onChange=this.mergeComponents
      }}
    {{/each}}

    {{yield this.mergedComponents}}
  {{/let}}
</@node>

wavesurfer/marker.js

export default class BouncePlayerWavesurferMarkerComponent extends Component {

  @action
  async didInsertParent(element) {
    this._element = element;
    await this._buildMarker.perform();
  }

  @action
  willDestroyParent(element) {
    this._destroyMarker.perform()
  }

  @action
  updateMarker(){
    this.displayMarker.setOptions({
      start: this.args.start,
      end: this.args.end
    })
  }

  _buildMarker = task(async () => {
    let regions = this.args.regions;
    // TODO: How can we synchronize things so that we don't have
    // to do this ugliness? Sometimes we fall into this block and
    // sometimes we don't.
    if(!regions){
      console.error('the regions are missing, try again later');
      later(() => {
        this._buildMarker.perform();
      });
      return;
    }

    this.displayMarker = await regions.addRegion({
      start: this.args.start,
      content: this.regionContent,
      color: 'rgba(0,0,255,0.5)',
      end: this.args.end || this.args.start + 0.20
    })

  })

  _destroyMarker = task(async () => {
    if(this.displayMarker){
      this.displayMarker.remove();
      this.displayMarker = null;
    }
  })

}

wavesurfer/marker.hbs

<@node
  @didInsertParent={{this.didInsertParent}}
  @willDestroyParent={{this.willDestroyParent}}
  ...attributes
  as |Node|
>
  <div class="hidden" {{did-update this.updateMarker @start @end}}>
    Hidden div for triggering updates since
    Node doesn't actually render anything.
  </div>
  {{yield}}
</@node>
miguelcobain commented 4 months ago

Firstly, make sure you need all the complexity for yielding an arbitrary amount of components. Ember-leaflet does come complex things to make sure that an ember-leaflet addon ("an addon for an addon") can registar new components that the main ember-leaflet component would then yield. If you don't need this feature, you can remove a lot of the code and just yield the components directly (without any componentsToYield, mergeComponents, etc).

Secondly, I think think them not returning a promise is a problem. On the contrary, it's a good thing.

The fact that you were initializing wavesurfer in a task might be cause some timing issues. I don't know. In any case, I'm proposing a simplification here, in which you access the wavesurfer instance via the parent (this.args.parent.wavesurfer). Passing wavesurfer directly might not work, because that property is not @tracked.

// bounce-player/wavesurfer.js

// ...

@action
didInsertParent(element) {
  this.wavesurfer = await WaveSurfer.create({
    container: element,
      // ... a bunch of other (probably) unrelated options
  });
}

@action
willDestroyParent(element) {
  // destroy this.wavesurfer using whatever methods it has for that purpose
}
{{! bounce-player/wavesurfer.hbs }}
<Root
  @didInsertParent={{this.didInsertParent}}
  @willDestroyParent={{this.willDestroyParent}}
  ...attributes
  as |Node|
>
  {{yield (hash
    timeline=(component "bounce-player/wave-surfer/regions" parent=this node=Node)
    regions=(component "bounce-player/wave-surfer/regions" parent=this node=Node)
  ))}}
</Root>
// wavesurfer/regions.js

@action
didInsertParent(element) {
  this.regions = RegionsPlugin.create({});
  this.args.parent.wavesurfer.registerPlugin(this.regions);
}
jagthedrummer commented 4 months ago

Thanks so much for the recommendations @miguelcobain! I had the feeling that cribbing ember-leaflet directly was leaving me with a bunch of stuff that I didn't really need, but I don't know enough about the inner workings to understand which of it I do need, which of it I don't need, and how to separate it. The simplifications you propose look great. (This is the sort of thing where would be helpful to have a "full example" in the docs.)

I'm about to duck out for the afternoon but will give this a shot later tonight and will report back.

Thanks again for the help, and for this excellent add-on! I think this is going to be a much better way of organizing all of this than the "one giant component managing everything directly in JS" approach that I'd taken previously.

jagthedrummer commented 4 months ago

Just spent some time with this and the unfortunately the suggestions didn't solve the problem.

The simplifications around componentsToYield are great! I have a much better idea now about how these pieces fit together. So thank you for that, @miguelcobain.

But moving the setup code out of a task and directly into didInsertParent didn't improve the situation, and actually seems like it may have made it more likely to encounter the timing issue.

In the interest of trying to untangle whether I have a wavesurfer.js problem or a ember-composability-tools problem (or both!) I tried to make a small example without wavesurfer.js to illustrate the issue.

Here's a parent component that uses a timeout (from ember-concurrency) in didInsertParent to simulate a long async setup in the parent. When didInsertParent fires in the child component the isReady flag in the parent will not be set with even as little as 1 ms of delay in the parent. In fact, even passing 0 to timeout will still cause the problem. You have to remove the timeout entirely for it to hit the happy path in the child component. (I'm guessing that await timeout(0) is about like doing later(...) in that it pushes the following code into the next iteration of the run loop, but the child setup is happening in this iteration.)

test/parent.hbs

<Root
  ...attributes
  @didInsertParent={{this.didInsertParent}}
  as |Node|
>
  <h3>The parent</h3>
  {{yield (component "Test/Child" node=Node parent=this)}}
</Root>

test/parent.js

import Component from '@glimmer/component';
import { action } from '@ember/object';
import { timeout } from 'ember-concurrency';
import { tracked } from '@glimmer/tracking';

export default class TestParentComponent extends Component {
  @tracked isReady = false;

  @action
  async didInsertParent(element) {
    console.log('Parent: didInsertParent')
    await timeout(10); // Simulate some setup that happens async
    this.isReady = true;
    console.log('Parent: isReady')
  }
}

test/child.hbs

<@node
  @didInsertParent={{this.didInsertParent}}
>
  <h3>The child</h3>
</@node>

test/child.js

import Component from '@glimmer/component';
import { action } from '@ember/object';

export default class TestChildComponent extends Component {
  @action
  didInsertParent(element) {
    console.log('Child: didInsertParent', this.args.id)
    if(this.args.parent.isReady){
      console.log('the parent is ready = ', this.args.parent.isReady);
    }else{
      console.error('the parent is NOT ready = ', this.args.parent.isReady);
    }
  }
}

test.hbs

<Test::Parent as |Test::Child|>
  <Test::Child/>
</Test::Parent>
jagthedrummer commented 4 months ago

I'm trying to work around the problem by having the parent notify it's children when it's finally ready, but this.children is always undefined for some reason.

In test/parent.js

  @action
  async didInsertParent(element) {
    console.log('Parent: didInsertParent')
    await timeout(1000);
    this.isReady = true;
    for(let child of this.children){
      child.parentIsReady();
    }
  }

When this runs it throws an error:

Uncaught (in promise) TypeError: this.children is not iterable
    at TestParentComponent.didInsertParent (parent.js:-43:27)

If I do console.log('this.children = ', this.children) it shows that this.children is undefined.

Even in actions fired after the components are setup and rendered (like on a button click or something) this.children is still undefined.

jagthedrummer commented 4 months ago

Seems like maybe the problem with this.children is because the parent component just uses Root in the template, which means the parent component isn't actually a Root itself, so it doesn't have any of the Root behavior.

I'm still pretty confused on why the docs say to extend Root when that doesn't seem to be the way that it's used in practice. When just using Root instead of extending it, is there a way to access the children?

(Is it possible that the async problems are due to not extending Root?)

jagthedrummer commented 4 months ago

I'm able get things to kind of work if I manually track a customChildren array in the parent and then have each child register themselves with this.args.parent during the child setup. But this feels like I'm rebuilding bits of ember-composability-tools that should be exposed to me. Am I missing something?

in test/parent.js

  @tracked customChildren = A([]);

  registerChild(child){
    this.customChildren.pushObject(child);
  }

  @action
  async didInsertParent(){
    console.log('Parent: didInsertParent', this.args.id)
    await timeout(1000);
    this.isReady = true;
    console.log('Parent isReady', this.customChildren, this);
    for(let child of this.customChildren){
      child.parentIsReady();
    }
  }

in test/child.js

  parentIsReady(){
    console.log('Child: parent is ready')
  }

  didInsertParentTask(){
    console.log('Child: didInsertParent')
    if(this.args.parent.isReady){
      console.log('the parent is ready = ', this.args.parent.isReady);
      this.parentIsReady();
    }else{
      console.error('the parent is NOT ready = ', this.args.parent.isReady);
      this.args.parent.registerChild(this);
    } 
  }
jagthedrummer commented 4 months ago

OK, I figured out how to have my component BE a Root and Node instead of having them only use those classes. Doing this means I don't have to manually register children, but the async problem still persists.

test/parent.hbs

<div
  {{!--
  These two actions are inherited from Root, but we need to call them
  manually since we're not using the Root template.
  --}}
  {{did-insert this.didInsertNode}}
  {{will-destroy this.willDestroyNode}}
  ...attributes
>
  {{yield (component "Test/Child" parent=this)}}
</div>

test/parent.js

import { Root } from 'ember-composability-tools';

export default class TestParentComponent extends Root {
  @tracked isReady = false;

  // this doesn't need to be an action since it's called directly by Root
  async didInsertParent(element) {
    console.log('Parent: didInsertParent')
    await timeout(1000);
    this.isReady = true;
    console.log('Parent: finally ready')
    for(let child of this.children){
      child.parentIsReady();
    }
  }
}

test/child.hbs

<div>
  <h3>The child</h3>
</div>

test/child.js

import { Node } from 'ember-composability-tools';

export default class TestChildComponent extends Node {
  parentIsReady(){
    console.log('Child: parent is ready')
  }

  // this doesn't need to be an action since it's called directly by Node
  didInsertParent(element) {
    console.log('Child: didInsertParent', this.args.id)
    if(this.args.parent.isReady){
      console.log('the parent is ready = ', this.args.parent.isReady);
      this.parentIsReady();
    }else{
      console.error('the parent is NOT ready = ', this.args.parent.isReady);
    }
  }
}