-
Notifications
You must be signed in to change notification settings - Fork 94
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 passing a callback to useFetch's run
to modify init
#76
Conversation
1b66fb5
to
ec58f61
Compare
Okay, now eslint should be happy, too. |
Okay, I've tried the failing |
Seems like the tests are generally breaking with react 16.9 and something |
I'm very happy with where you're going with this. I'm in doubt about the callback though. I like the flexibility it provides but I still would prefer to have the option to just pass an object. We can detect a SyntheticEvent using one of the attributes they have, such as My preference would be:
|
Sounds like a plan! I'll do that as soon as I'm back.
Am 10. August 2019 13:41:34 MESZ schrieb Gert Hengeveld <[email protected]>:
…I'm very happy with where you're going with this. I'm in doubt about
the callback though. I like the flexibility it provides but I still
would prefer to have the option to just pass an object. We can detect a
SyntheticEvent using one of the
[attributes](https://reactjs.org/docs/events.html#overview) they have,
such as `preventDefault`.
My preference would be:
- if function, use it to determine init
- if object and not SyntheticEvent, spread it after init
- else do nothing with init
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
|
bae616c
to
db67da9
Compare
Okay, that should do it for the functionality. I also pushed a commit that gives And I added an example. But unfortunately, I can't get the examples to run - are these examples currently runnable? It seems as if it can't parse the jsx tokens in |
0e5ccae
to
169c75a
Compare
I just rebased this on master and it seems to fail again - I guess this is due to some change on master? |
Yes the introduction of React v16.9 is causing some issues. On CircleCI things work fine but on Travis they fail. I suspect updating the examples to 16.9 might help. |
That seems to work. Should be fixed now. |
Codecov Report
@@ Coverage Diff @@
## master #76 +/- ##
==========================================
+ Coverage 98.91% 98.93% +0.01%
==========================================
Files 8 8
Lines 370 375 +5
Branches 136 140 +4
==========================================
+ Hits 366 371 +5
Misses 4 4
Continue to review full report at Codecov.
|
Okay, tests are now passing. |
6340b08
to
cae8622
Compare
Released in v8.0.0 |
@all-contributors please add @phryneas for ideas |
I've put up a pull request to add @phryneas! 🎉 |
Heyo :)
This adresses #75.
Instead of using a parameter to be spread to init, I chose for a callback function.
This has two reasons
run
is just passed as an onClick handler, a React SyntheticEvent would have been passed and possibly spread on toinit
. Detecting all kinds of events would be a pain, buttypeof x === "function"
is a simple test that it is NOT an event.deep-merge
I also split up the method definition between
deferFn
andpromiseFn
because the handling of 2 parameters vs 3 parameters was already confusing enough (sometimes props is ctrl, sometimes ctrl is ctrl).I think this improves readability without impacting code size too much.
Also, I reduced boolean logic at two places to better handle the
undefined
case ofdefer
andjson
.Of, and of course there's a new unit test and a bit more README :)
Looking forward to your feedback. 🙋