Rewrite: useReducer based implementation

[dai-shi]

https://twitter.com/dai_shi/status/1185849112895225856

In react-hooks-async, I tried useReducer but it was too ugly, the current code with forceUpdate is pretty readable. So, I gave up. I could try further. There could be a better implementation with only immutable states. I wish somebody would try.

https://twitter.com/dai_shi/status/1191518644389335040

1. react-hooks-async implementation is not very declarative.

I actually agree. Used to useReducer, but changed assuming mutations are easier to read. I'll try again. It should be technically possible

So, it should be technically possible to code without forceUpdate.
It would be better because forceUpdate is not a good pattern unless there's an avoidable reason.

Now, I hope it's not too ugly now. I don't preserve the exact same behavior(no abort in this hook), but the public API should be able to be used the same as before.

There might be room for improvements. a) dispatchRef doesn't seem nice, b) do we really need two symbols? c) Is the use of useLayoutEffect correct?

I found implementing this is pretty tricky, and I'm not sure how readable it it.

[dai-shi]

@dagda1 You wanna have a look?

[dagda1]

HI @dai-shi,

I feel really awful. I was not meaning to criticise. I was just saying that somebody in the comments reminded me of react's declarative nature. I learned a lot looking at this library and I think it is excellent.

I will have a look at the PR

[dai-shi]

I was not meaning to criticise.

@dagda1 Please don't get it wrong. I never felt this as criticism. It's a suggestive feedback. I mean I thought the use of forceUpdate wasn't nice. It should be a last resort. I had wanted to rewrite this.

My implementation was originally useReducer based, but when I supported the "callback" pattern, I changed it to forceUpdate+memoization. Now, I got to know useReducer better, I'm using lazy initialization this time.

[dagda1]

I think this is much better but the one thing I don't see is a state change for an Error.

I like the reducer in the codesandbox I referenced in the article.

[dai-shi]

      let result = null;  
      let error = null;  
      try {  
        result = await func(abortController, ...args);  
        dispatchRef.current({                                                 
          type: 'RESULT',                                                     
          taskId,  
          runId,   
          result,  
        });  
      } catch (e) {  
        if (e.name !== 'AbortError') {  
          error = e;  
        }  
        dispatchRef.current({  
          type: 'ERROR',                                                      
          taskId,                                                             
          runId,                
          error,  
        });  
      }  
      if (error) throw error;  
      return result;  

You mean you prefer this?

[dagda1]

yes,an error is a state change IMO

On Tue, 5 Nov 2019 at 21:53, Daishi Kato ***@***.***> wrote: let result = null; let error = null; try { result = await func(abortController, ...args); dispatchRef.current({ type: 'RESULT', taskId, runId, result, }); } catch (e) { if (e.name !== 'AbortError') { error = e; } dispatchRef.current({ type: 'ERROR', taskId, runId, error, }); } if (error) throw error; return result; You mean you prefer this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33?email_source=notifications&email_token=AAA44ODRSFIABLUESQCTC4DQSHTM7A5CNFSM4JJDRXVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDEO3GI#issuecomment-550038937>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA44OFZK65RUW4XESWGVFDQSHTM7ANCNFSM4JJDRXVA> .

-- Cheers Paul Cowan Cutting-Edge Solutions (Scotland) blog: http://thesoftwaresimpleton.com/ website: https://cutting.scot/

[dai-shi]

@dagda1

yes,an error is a state change IMO

But, it was already a state change. I think it's just coding style preference.

[dagda1]

should abort be a state changed also?

[dai-shi]

Hm, currently, we have no aborting nor aborted state, because abort() is usually called on unmount or on starting a new task and no component read the state. If we had a use case to use aborting state, we would consider that. How does that sound?

[dagda1]

i did have a use case recently but it was for uploading large files into azure in chunks. are you saying abort is only called on unmount

[dai-shi]

No, I only meant "usually". Let's open a new issue about it. I think it's a good use case to dig in.

[dai-shi]

Let me merge this.

@dagda1 Would you open a new issue for aborting state with a use case?

[dai-shi]

should abort be a state changed also?

Actually, I ran into an issue because of no aborted state. There is a bug in the current typings after #30.

[alvinthen]

Hey, FYI, useLayoutEffect is all fine except that it gives all bunch of warnings in SSR

[dai-shi]

@alvinthen You are right. That's a really undesired behavior of React.
Fair enough. Let's see how useIsomorphicLayoutEffect hack works.