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

Allow using taskFor at assignment #1

Merged

Conversation

jamescdavis
Copy link
Collaborator

This adjusts the assertion on the argument passed to taskFor such that it can be used at assignment, e.g.

@task
myTask = taskFor(function*() {
  yield 1;
  return 2;
});

@@ -3,7 +3,7 @@ import { assert } from '@ember/debug';
export function taskFor(task) {
assert(
`${task} does not appear to be a task!`,
task && typeof task.perform === 'function'
task && (typeof task === 'function' || typeof task.perform === 'function')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work for encapsulated tasks. Unfortunately, if we want this to work I think we probably just have to give up on having an assertion at all. The original intent for this is to guard the fact that, despite allowed by the types, passing a normal generator or async function here does not magically turn it into a task, and this really only works if you have the decorator pre-applied. Now if we are purposefully allowing that, I think we'll just have to remove the assertion. We could update it at check specifically for the kind of functions or POJOs that are allowed by the decorator, but at that point we would just be duplicating what TypeScript already checked for.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually works just fine because an encapsulated task descriptor is an object with a function named perform. I've added a test proving so. 😄

Copy link
Owner

@chancancode chancancode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I am okay with this, but can you add tests for encapsulated task in assignment usage, as well as mentioning this in the README as an alternative?

@jamescdavis
Copy link
Collaborator Author

jamescdavis commented Jun 25, 2020

@chancancode as a first step: #3

If you want to get that in first, I'll rebase this.

@jamescdavis jamescdavis force-pushed the allow_using_taskFor_at_assignment branch from 19d4692 to d7bddd6 Compare June 25, 2020 11:06
@jamescdavis
Copy link
Collaborator Author

Ok, I've added the encapsulated task test and documented this usage in the README.

Once chancancode/ember-concurrency-async#7 is released, I'll go back and update these docs to add:

@task myTask = taskFor(async () => {
  await this.something;
  return 'done!';
}

to show how using an async arrow function removes the need to assert the type of this.

Copy link
Owner

@chancancode chancancode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #3 is almost good to merge other than fixing one import issue, let's merge that and rebase this on top

@chancancode
Copy link
Owner

Merged #3, this will need a rebase

@jamescdavis jamescdavis force-pushed the allow_using_taskFor_at_assignment branch from 55c61ed to ee3a4fc Compare July 6, 2020 15:13
@jamescdavis
Copy link
Collaborator Author

@chancancode rebased

@chancancode chancancode merged commit bbad2fb into chancancode:main Jul 6, 2020
@chancancode
Copy link
Owner

Thank you! Also you can merge/push/release now. 😛 To release, just run yarn version locally and push the tag/commit and CI will take care of the rest (don't forget to push the commit to the branch too).

@jamescdavis jamescdavis deleted the allow_using_taskFor_at_assignment branch July 7, 2020 14:08
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.

2 participants