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

RFC: Public Yieldables API #413

Merged
merged 9 commits into from
Apr 9, 2021
Merged

RFC: Public Yieldables API #413

merged 9 commits into from
Apr 9, 2021

Conversation

maxfierke
Copy link
Collaborator

@maxfierke maxfierke commented Mar 8, 2021

Rendered

Implementation is included as well, which shows refactor of built-in Yieldables to use the new API. Refactoring (or similar) will probably stand, regardless of whether to promote to public API, as calling taskInstance.proceed with various magic values in a bunch of places is error prone.

@maxfierke maxfierke added rfc Request for comments on feature proposal v2 Applies to ember-concurrency v2 labels Mar 8, 2021
rfcs/0000-yieldables.md Outdated Show resolved Hide resolved
@machty
Copy link
Owner

machty commented Mar 10, 2021

Looks great! Glad you're making this more public. I like that some of the ugly internals for "resuming" within the yieldable are hidden.

One thing though:

In general, Yieldable instances should not be reused across calls

I would change this so that you can resume a yieldable instance across calls. A Yieldable should be more like a stateless "description" of the async operation you want to build when e-c processes the Yieldable you yield to it. This is the basic idea behind Rx Observables: the observable objects don't have any state on them until you .subscribe() to it, and it allows for patterns of composability that are pretty neat (i.e. transform/combine a bunch of observables into one single observable that you subscribe to). I'm not sure those same composability benefits would come to the Yieldables API in the same way as it does for observables, but it seems like a better foundation to build on then one where yieldables can't be reused between yields.

From what I gather, I don't think making this change would actually change the public API for building yieldables, and it has the nice benefit that you would be able to reuse yieldable instances across calls, e.g.

const sharedTimeout = timeout(500);

// ...

yield sharedTimeout;
// do stuff
yield sharedTimeout;

vs having to wrap sharedTimeout in a function.

@maxfierke
Copy link
Collaborator Author

Looks great! Glad you're making this more public. I like that some of the ugly internals for "resuming" within the yieldable are hidden.

One thing though:

In general, Yieldable instances should not be reused across calls

I would change this so that you can resume a yieldable instance across calls. [...]

From what I gather, I don't think making this change would actually change the public API for building yieldables, and it has the nice benefit that you would be able to reuse yieldable instances across calls, e.g.

const sharedTimeout = timeout(500);

// ...

yield sharedTimeout;
// do stuff
yield sharedTimeout;

vs having to wrap sharedTimeout in a function.

@machty I think I might have phrased this weirdly... it is safe to use across calls within the same TaskInstance execution, but not across different TaskInstances. i.e. yes, you can make one call to timeout and yield that multiple times within the same task instance, but you could not, say, do the following:

const myTimeout = timeout(500);

class MyClass { 
  @task *aTask() {
    yield myTimeout
  }
  
  @task *bTask() {
     yield myTimeout;
   }
}

^ This would not be allowed.

If we want to allow that, I think we can do that, although the API might need to be a little bit different, especially to accomidate Promise casting of yieldables (which I just realized I forgot to define.)

This is actually how Yieldables worked internally before and makes it easier to
avoid sharing taskInstance at the class-level, making suitable for cross-call,
and cross-task use of a single instance of a Yieldable
@MLeganes

This comment has been minimized.

@maxfierke

This comment has been minimized.

@maxfierke
Copy link
Collaborator Author

@machty Revised the RFC last night a little bit in order to introduce YieldableState to keep the TaskInstance invocation state separate from the Yieldable instance's state. Would be curious if you have any additional thoughts?

Otherwise I think I'll probably move forward with this approach.

@maxfierke maxfierke merged commit c37e383 into master Apr 9, 2021
@maxfierke maxfierke added this to the 2.1.0 milestone May 31, 2021
@maxfierke maxfierke deleted the mf-public_yieldable branch June 16, 2021 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Request for comments on feature proposal v2 Applies to ember-concurrency v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants