diegohaz / redux-saga-thunk

Dispatching an action handled by redux-saga returns promise
MIT License
221 stars 17 forks source link

Optional thunk identifier #24

Closed braska closed 6 years ago

braska commented 6 years ago

Resolves #20

codecov-io commented 6 years ago

Codecov Report

Merging #24 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #24   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines          80    112   +32     
  Branches       25     39   +14     
=====================================
+ Hits           80    112   +32
Impacted Files Coverage Δ
src/utils.js 100% <100%> (ø) :arrow_up:
src/selectors.js 100% <100%> (ø) :arrow_up:
src/middleware.js 100% <100%> (ø) :arrow_up:
src/reducer.js 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e250bb0...cbe3b94. Read the comment docs.

diegohaz commented 6 years ago

What an amazing job, @braska. Thank you.

I'll take a time this weekend to look at the code.

braska commented 6 years ago

I am worried only about one thing: in original version meta.thunk become string after passing middleware (e.g.: FOO_1234567891234567_REQUEST), but now middleware puts object to this field ({ name: 'FOO', key: '1234567891234567', type: 'REQUEST' }). I hope that the users of this package don't interact with the string in meta.thunk.

diegohaz commented 6 years ago

I hope that the users of this package don't interact with the string in meta.thunk.

They are not supposed to. However, I'm gonna release it as v0.7.0, just in case.

One question: what are the API for defining the thunk id? Are these two snippets valid?

const action = {
  type: 'MY_ACTION',
  payload: { id: 'foo' },
  meta: { thunk: 'foo' },
}
const action = {
  type: 'MY_ACTION',
  payload: { id: 'foo' },
  meta: { thunk: { id: 'foo' } },
}
braska commented 6 years ago

Only second action is valid. I was thinking about the first option, but realized that payload.id not always acceptable for using as thunk identifier.

diegohaz commented 6 years ago

Okay, let's merge it. We can improve it later if needed. Thank you very much, @braska.

Since you've played a lot with the code, do you want to become a collaborator on this project?

braska commented 6 years ago

It sounds very cool! I can't promise that I will always have time to do something in this repo, but I'm ready to help if possible.

diegohaz commented 6 years ago

Awesome. Don't worry. It's just so you have more permissions in the case you need to.