search

machty / ember-concurrency-decorators

Task decorators for ember-concurrency 1.x (merged into ember-concurrency 2.0)
MIT License
70 stars 25 forks source link

feat: add `last-value` decorator #13

Closed alexlafroscia closed 5 years ago

alexlafroscia commented 6 years ago

Closes #12

alexlafroscia commented 6 years ago

Not sure if you'll be able to access this page, but the tests are passing now on Travis with the removal of those ember-try cases that didn't actually have a configuration:

https://travis-ci.com/alexlafroscia/ember-concurrency-decorators/builds/86374121

buschtoens commented 6 years ago

Sorry for letting this sit around so long. The notification must have slipped past me.

Looking great already! What do you think about using an initializer instead of a default / defaultFactory option?

@lastValue('someTask')
value = [];

The cool thing is, that desc.initializer already is a (factory) function.

alexlafroscia commented 6 years ago

Totally, that makes way more sense!

Should the semantics around the "default" changing slightly then? Right now it's "use the default (or factory) if the task has not been run or if the task returned undefined or null". If we're using the initializer instead, it might make more sense for it only to use that value if the task hasn't finished yet.

buschtoens commented 6 years ago

Maybe it's personal preference, but I always write tasks in a way that they either return a value that can be handled by the receiver or else throw an error. From that POV I would expect the decorator to only return the default value, if there is no lastSuccessful yet.

However this might just be me / us. We could add a defaultIfNone / defaultIfNullish (naming things is hard, let's bikeshed :D ) option:

@lastValue('someTask', { defaultIfNone: true })
value = [];
buschtoens commented 6 years ago

A second thing: I think it makes total sense that @lastValue reads lastSucessful.value, as in most cases you only care about successful tasks. But we could (potentially in a later PR, so we can :shipit: now) add another option like:

@lastValue('someTask', { onlySuccessful: false })
value = [];
alexlafroscia commented 6 years ago

I always write tasks in a way that they either return a value that can be handled by the receiver or else throw an error

Same here; I think that's reasonable advice to give to people using ember-concurrency as well.

I think we should start with the most basic API, like you suggested initially, where the initializer provides the default value. We can expand the API with more options and stuff later on, but it may not be necessary. I'm definitely of the mind to keep it simple early on and let people request features 😛

buschtoens commented 5 years ago

Out of curiosity: what auto-formatter are you using? Prettier?

alexlafroscia commented 5 years ago

Yeah, prettier. Sorry about that, I can remove the bits it changed in the README. I’ve set up my editor to format on save and haven’t looked back; I was skeptical at first but really like it now.

Or, if you’d like, I’m happy to PR a Prettier config that ties into ESLint if you want to go that direction.

buschtoens commented 5 years ago

No need to be sorry. 😊

I’m happy to PR a Prettier config that ties into ESLint if you want to go that direction.

That would be extra sweet! Didn't get around to it yet unfortunately. Can you please set singleQuotes then?

alexlafroscia commented 5 years ago

Just re-based the PR to fix your minor performance note and fix the yarn.lock merge conflict

I'm happy to do the prettifying; I'll do that in a separate PR it ended up just taking a second and I prettied the files I added, so it was easier to just add to this PR.

alexlafroscia commented 5 years ago

Passing build can be seen on my fork here:

https://travis-ci.com/alexlafroscia/ember-concurrency-decorators/builds/87582000?utm_medium=notification&utm_source=email

If you're interested in adding a maintain, I'd be happy to get Travis configured for you on this repo! It can integration with the new "Checks" tab on pull requests, it's pretty sweet.

alexlafroscia commented 5 years ago

I also have a PR ready for when this one is merged that runs ember-cli-update and updates a bunch of dependencies!

I want to make that as a separate PR after this lands because there would have been conflicts between this and that branch and it was easier to just update from this branch as the starting point.

buschtoens commented 5 years ago

If you're interested in adding a maintain, I'd be happy to get Travis configured for you on this repo! It can integration with the new "Checks" tab on pull requests, it's pretty sweet.

Yes, I'd love to have you onboard! Thank you for offering it. I've pinged @machty, so that he can add you. 🙂