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

Make the addon promise aware #58

Closed

Conversation

jagthedrummer
Copy link

@jagthedrummer jagthedrummer commented Feb 20, 2024

This makes it so that asyncronous setup in a parent component will be awaited before trying to setup child components.

If any given didInsertNode/didInsertParent functions are NOT marked with async then I think this change should have no effect on them.

Fixes #57

Example usage:

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
>
  <div>The parent</div>
  {{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', this.args.id);
    // This simulates an async setup process
    await timeout(this.args.setupTime || 0);
    this.isReady = true;
    console.log('Parent: finally ready', this.args.id);
  }
}

test/child.hbs

<div>The child</div>

test/child.js

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

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

  // this doesn't need to be an action since it's called directly by Node
  didInsertParent(element) {
    console.log('Child: didInsertParent', this.args.id, this.args.parent.isReady);
  }
}

test.hbs

<Test::Parent @id="p1" @setupTime={{1000}} as |Test::Child|>
  <Test::Child @id="c1.1"/>
  <Test::Child @id="c1.2"/>
</Test::Parent>

<Test::Parent @id="p2" @setupTime={{100}} as |Test::Child|>
  <Test::Child @id="c2.1"/>
  <Test::Child @id="c2.2"/>
</Test::Parent>

Console output with this PR:

// immediately
Parent: didInsertParent p1
Parent: didInsertParent p2

// after 100 ms
Parent: finally ready p2
Child: didInsertParent c2.1
Child: parent is ready c2.1
Child: didInsertParent c2.2
Child: parent is ready c2.2

// after 1000 ms
Parent: finally ready p1
Child: didInsertParent c1.1
Child: parent is ready c1.1
Child: didInsertParent c1.2
Child: parent is ready c1.2

Console output without this PR:

// immediately
Parent: didInsertParent p1
Child: didInsertParent c1.1
Child: didInsertParent c1.2
Parent: didInsertParent p2
Child: didInsertParent c2.1
Child: didInsertParent c2.2

// after 100 ms
Parent: finally ready p2

// after 1000 ms
Parent: finally ready p1

This makes it so that asyncronous setup in a parent component
will be `await`ed before trying to setup child components.

If any given `didInsertNode` and `didInsertParent` functions are NOT marked
with `async` they I _think_ this change should have no effect on them.
@jagthedrummer jagthedrummer changed the title Make the addon-on promise aware Make the addon promise aware Feb 20, 2024
@jagthedrummer
Copy link
Author

@miguelcobain, just curious if you have a feel for whether or not this is a safe change. It seems to be working for me like I'd expect. I'm almost ready to deploy a new version of my project that's using it, and I'm wondering if I'll be able to use the regular add-on of if I'll need to set things up to use my own fork.

@jagthedrummer
Copy link
Author

I see that there are a couple of test failures here. One looks like it may be a legit failure related to this PR, and the other seems to be a dependency issue of some sort. I'm running into that same dependency issue locally when I try to run the test suite, both on my branch and on mater, so I'm not sure how to even investigate the other failure at the moment.

$ ember test
====================================================================

  The `ember` node module is a placeholder, you may be looking for:

  * `ember-cli` (the command line tool)
  * `ember-source` (the framework code)

  Visit https://emberjs.com/ for more details

====================================================================

  Forwarding request to ember-cli via `npx ember-cli test`
Environment: test
cleaning up...
Build Error (FastBootConfig)

The comparison function must be either a function or undefined


Stack Trace and Error Report: /var/folders/fx/vmqbtyws34j1l88m16_9jy_00000gn/T/error.dump.749d2e4a685fa1a4e525caab63bbd8fc.log

I think this is the relevant portion of that log:

  - broccoliBuilderErrorStack: TypeError: The comparison function must be either a function or undefined
    at Array.sort (<anonymous>)
    at stringify (/Users/jgreen/projects/ember-composability-tools/node_modules/json-stable-stringify/index.js:73:31)
    at stableStringify (/Users/jgreen/projects/ember-composability-tools/node_modules/json-stable-stringify/index.js:90:3)
    at FastBootConfig.toJSONString (/Users/jgreen/projects/ember-composability-tools/node_modules/ember-cli-fastboot/lib/broccoli/fastboot-config.js:170:12)
    at FastBootConfig.build (/Users/jgreen/projects/ember-composability-tools/node_modules/ember-cli-fastboot/lib/broccoli/fastboot-config.js:56:53)
    at TransformNodeWrapper.build (/Users/jgreen/projects/ember-composability-tools/node_modules/broccoli/dist/wrappers/transform-node.js:71:39)
    at /Users/jgreen/projects/ember-composability-tools/node_modules/broccoli/dist/builder.js:185:30
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Builder.build (/Users/jgreen/projects/ember-composability-tools/node_modules/broccoli/dist/builder.js:204:13)
    at async Builder.build (/Users/jgreen/projects/ember-composability-tools/node_modules/ember-cli/lib/models/builder.js:204:11)

@miguelcobain
Copy link
Owner

Closed in favor of #59

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.

Advice for dealing with ansync setup in parent components?
2 participants