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

@redux-requests/react: ordinary event passed into dispatchRequest produces typescript error #433

Closed
avasuro opened this issue Jan 20, 2021 · 13 comments

Comments

@avasuro
Copy link

avasuro commented Jan 20, 2021

Actual behaviour

TypeScript throws type error when non-RequestAction instance is passed into dispatchRequest function.

Expected behaviour

TypeScript doesn't throw types error when non-RequestAction is passed into dispatchRequest function

Bug description:

According to documentation dispatchRequest function is just a wrapper for native redux dispatch function, and used only to add proper type checks. From my point of view that should mean that his function should be able to accept RequestAction as well as any other ordinary redux action. And it does accepts, but because of wrong typescript typings for that function typescript fails with error if I try to pass any non-RequestAction instance to this function.

Code example

Live example
_(this code will not work because react hook is used outside any react component, redux is not initialised etc. - I tried to make code as short as possible to focus only on main issue - typescript error)

In case of live example not works - there is the source code:

import { useDispatchRequest } from "@redux-requests/react";
import { RequestAction } from "@redux-requests/core";

// Request action creator:
export const createRequestAction = (): RequestAction => ({
  type: "SOME_REQUEST_ACTION",
  payload: {
    request: {
      url: "google.com"
    }
  }
});

// Normal action creator:
export const createNormalAction = () => ({
  type: "SOME_NORMAL_ACTION",
  payload: "any payload for normal action"
});

// eslint-disable-next-line
const dispatch = useDispatchRequest();



// Example #1 - pass any normal action produces typescript error:
dispatch(createNormalAction());

// Example #1 - pass RequestAction works fine:
dispatch(createRequestAction());
@klis87
Copy link
Owner

klis87 commented Jan 20, 2021

@avasuro hmm, indeed, it is a wrapper from js perspective, but it changes its type from ts perspective. I don't think it should support custom Redux action though, because it also makes dispatch returning response promise. However, normal dispatch returns just dispatched action. Would it be ok to leave it like that, but maybe some extra info should be added to docs? Then, you would need to import both useDispatchRequest and standard useDispatch when using TS.

@klis87
Copy link
Owner

klis87 commented Jan 20, 2021

you could also create a wrapper of yours for this hook, which could work like const [dispatchRequest, dispatch] = useWrapperDispatchRequest()

@klis87
Copy link
Owner

klis87 commented Jan 20, 2021

Anyway, with the newest version, it is very rare to dispatch request actions manually, see useQuery autoload, also hooks response contains memoized callback to dispatch, like load and mutate

@avasuro
Copy link
Author

avasuro commented Jan 20, 2021

@klis87
Thank you for your quick response!

I'm not 100% sure but from my point of view it is worth to make it accept ordinary actions, because:

  • it not looks very good to import several dispatch methods
  • End user have to pay his attention at which action is a RequestAction, and which is not to use proper dispatch method - that may be annoying
  • This function also unable to accept actions produced by this library (like abortRequest and resetRequest) - so end used should keep in mind that too
  • About useQuery and autoload - that's true, but may not work for all projects. Some may want to avoid using useQuery to keep "old fashion" way of dispatching actions and using selectors, because such approach allows to replace this library in future, and that will not affect (in theory) any application component - all changes will be done in redux part.

@klis87
Copy link
Owner

klis87 commented Jan 20, 2021

Thanks for your valid points, probably we could use TS conditional types to make it work for both cases, do you feel like creating a PR? ;)

@avasuro
Copy link
Author

avasuro commented Jan 20, 2021

@klis87

I proposed a PR with possible fix, please take a look, and make your decision about expected usage of dispatchRequest method basing on our discussion and this PR.
If you decide to decline this proposal, then I think it will be good to better describe in documentation how dispatchRequest should be used in projects (like users should import two dispatch requests etc).

Thank you for you efforts, I use new version of this library first time and i like it, API become better :)

@klis87
Copy link
Owner

klis87 commented Jan 20, 2021

Thx, I will take a look! Also, if you have time, I would appreciate your opinions in issues labeled as advice wanted especially #432

@klis87
Copy link
Owner

klis87 commented Apr 3, 2021

@avasuro The next version won't have dispatchRequest at all, just normal dispatch. It will accept any action, and only for request actions it will promisify response. I will look more or less like:

export interface Dispatch {
  <Action = AnyAction>(action: Action): Action extends RequestAction<
    any,
    infer Data
  >
    ? Promise<{
        data?: Data;
        error?: any;
        isAborted?: true;
        action: any;
      }>
    : Action;
}

interface RequestsStore extends Store {
  dispatch: Dispatch;
}

So as you can see, store will just have dispatch, like normally, but thx to conditional types, It will correctly handle requests.

@avasuro
Copy link
Author

avasuro commented Apr 4, 2021

@klis87 Thank you, very good solution!

@john-wennstrom
Copy link

john-wennstrom commented Apr 16, 2021

Hello @klis87 . I'm having problems understanding the useDispatchRequest hook. In my case I would like to dispatch an action and await the response so I can do something with the error in the same function where I dispatch, like this:

import { useDispatchRequest } from "@redux-requests/react";
...

const CreateCase: React.FC<CreateCaseProps> = (props) => {
   const dispatch = useDispatchRequest();

   const handleSubmit = async (data: any) => {
      const { data, error } = await dispatch(createCase(data));
      if (error) {
         ...
      }
   }

   return <Button onClick={() => handleSubmit(caseForm)}>Create</Button>;
}

I get a Unhandled Rejection (TypeError): Cannot convert undefined or null to object in create-send-requests-middleware.js:245

I cannot see what I'm doing wrong here, any suggestions would be appreciated.

@klis87
Copy link
Owner

klis87 commented Apr 16, 2021

@john-wennstrom useDispatchRequest is just reexport of useDispatch from react-redux, just types are different as dispatch returns promise, while normal dispatch returns just action itself.

The only reason dispatch could reject promise is a syntax error, probably in your request action. Please show me createCase

@john-wennstrom
Copy link

@klis87 Yes, I now see why it didn't work. Forgot to return something from the request action.

@klis87
Copy link
Owner

klis87 commented Apr 21, 2021

@john-wennstrom ok, good you found the problem, co closing then

@klis87 klis87 closed this as completed Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants