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

Request creators #432

Open
klis87 opened this issue Jan 13, 2021 · 14 comments
Open

Request creators #432

klis87 opened this issue Jan 13, 2021 · 14 comments

Comments

@klis87
Copy link
Owner

klis87 commented Jan 13, 2021

Right now we create requests like this:

const fetchBook = id => ({
  type: 'FETCH_BOOK',
  request: { url: `/books/${id}` },
  meta: { requestKey: id },
});

This is problematic due to the following reasons:

  1. no autocomplete, unless you use TS and RequestAction interface, but with pure JS you won't get autocomplete at all
  2. it is just redux action, it requires to have constant and action separately, however often we need both to a function like useQuery, type is needed for selector and action is needed to be called to automatically fetch
  3. issue from point 2 is of course solvable by using action creator library, which adds constant automatically and adds toString method returning this type, so that action becomes both action itself and source of its type, I mean libraries like redux-act, redux-actions and my redux-smart-actions
  4. but 3 is hard to explain to many new Redux developers, and probably just impossible totally for people not knowing Redux, and asking to use constants looks pretty bad, API won't look attractive, and telling people to use action creator library will also look weird, especially the goal is to use this lib even by people not knowing Redux, right now it is already possible with the latest React release

So, suggestion? For instance helper like createQuery, which could work like that:

const fetchBook = createQuery(
  'FETCH_BOOK',
  id => ({ url: `/books/${id}` }), // driverConfig, will go to `request`
  id => ({ requestKey: id }), // requestOptions, will go to `meta`
}));

Then, fetchBook(1) will return the same what previous version, but fetchBook.toString() === 'FETCH_BOOK', so no constants anymore.

What we gain?
5) autocomplete
6) No reason to explain why we need constants for people not knowing Redux
7) We could do nice stuff, like inject meta.variables which might be nice source of info, we know also whether request is query or mutation, so we won't need things like meta.asQuery anymore
8) createQuery etc could have some generics, which could be filled with drivers like axios driver, then you could import createQuery from a driver and also have driver config typed and autocompleted

This would be not breakable, current way of writing would also work, but in the next major version:
9) API simplification, useQuery won't have action prop anymore, as type will be always action creator, never string
10) I would really remove support for FSA action, no payload.request, just request, this really shouldn't matter, and library could be simplified

But are there any disadvantages? I cannot see, but maybe I miss some, please write if anything comes to your mind!

@iwan-uschka
Copy link

Hi @klis87,
i totally understand the motivation here. Questions:

  1. useQuery syntax would look like
    const myQuery = useQuery({ type: someAction.toString() });
    
    ?
  2. What is meant with meta.variables at point 7? I just 😱😱😱 discovered the property variables for useQuery. Is this related?
  3. I am not sure if i understand point 8 correctly. If i am suppose to declare (and optionally configure) drivers globally like described at https://redux-requests.klisiczynski.com/docs/drivers/using-drivers what's the idea behind creating a query action independently from the drivers being set globally? Do you think of a shortcut here and the possibility of not having to declare drivers in general? If that's the case i think that's a good idea to lower the barriers and seeing the global driver declaration as an optional feature, beginners (or anybody) don't have to care about.

Answers/opinions:

  1. I wouldn't throw away FSA compatibility for the sake of interoperability. As far as i know FSA is the most common standard (funny that this sentence implicates the existence of more than a single standard :) ). If you need to use some FSA-only-middleware you would need to write your own middleware wrapper-adapter-thing for it. I don't like having to use payload.request but i like the fact that i know what to expect when i see and handle payload property. Moving away from a standard results in an increasing learning curve (probably).

@klis87
Copy link
Owner Author

klis87 commented Jan 17, 2021

@iwan-uschka

  1. no, just useQuery({ type: someAction }) (instead of useQuery({ type: 'SOME_ACTION' })), this is already possible in the current version, it is also possible that the next major release will make it as useQuery(someAction, options), as type is necessary always anyway, options are optional though
  2. variables were added recently, see React guide in the docs, variables are useful when autoLoad is true, and your query action accepts some props, then useQuery will inject them when calling type callback (to dispatch query action)
  3. this is about having better Typescript types and autocomplete in editor for request action, right now we have autocomplete for meta, which are general options for the core, but request depends on driver, be it axios config, fetch config, or whatever a driver expects. We could add generic DriverOptions to createQuery and its family for example, which could be then set accordingly depending on used driver (like axios config)

Regarding FSA, this is really a tough call, I thought about this because API is growing, lib becomes more and more complex and this could simplify things a lot in many places. Plus, with this library, really you almost don't need to use Redux at all, and my goal is to make it clear that no Redux knowledge is required to use it, but thx to Redux used for the core, apps are easy to debug, we have access to stable ecosystem and we can track any state transition in Redux dev tools for free.

Regarding FSA-only middleware, are you aware of any popular which assumes only FSA action types?

@iwan-uschka
Copy link

Thanks for your response.

  1. useQuery({ type: someAction }) => pretty cool
    useQuery(someAction) => even nicer
  2. will check out `variables``
  3. I totally get that you can use driver specific createRequest function to benefit type checking and autocompletion. But i am still not quite sure about whether it interferes with declaring and configuring drivers globally via handleRequests and createDriver.

Regarding FSA-only middleware, are you aware of any popular which assumes only FSA action types?

Nope :)

@klis87
Copy link
Owner Author

klis87 commented Jan 17, 2021

  1. No, it won't make any difference at all regarding drivers setup in handleRequests and createDriver

@avasuro
Copy link

avasuro commented Jan 20, 2021

I agree with all points, and totally agree that current necessity to provide action type together with action creator (in default setup without help of other action creator libraries) looks very ugly - request creator should gracefully solve that issue.

Just want to say 2 things:

  • please, don't drop FSA - I know at least one lib ( @reduxjs/toolkit ) which method createAction produces FSA action
  • about variables - it's not always possible to know in ahead which variables will be passed into useQuery or useMutation hook, so it will be good to have a way to pass variables directly into load method (that will be true for cases when autoload = false in useQuery). Example:
const {load} = useQuery({
    type: 'IS_EMAIL_REGISTERED',
    action: isEmailRegistered,
    autoload: false
});

// This is submit handler for form provided by "formik" library:
const onFormSubmit = (formValues, formHelpers) => {
    // Before user registration - check whether user with such email already registered,
    // and if yes - display proper error below email input:
    const { data: isEmailAlreadyRegistered } = await load(formValues.email); // <-- HERE IS VARIABLES PASSED
    if (isEmailAlreadyRegistered) {
        formHelpers.setFieldError('email', 'Email already registered');
        return;
    }
   
    // ... Email is not registered, so make request here to register user
}

@klis87
Copy link
Owner Author

klis87 commented Jan 20, 2021

@avasuro thx for the great feedback!

please, don't drop FSA - I know at least one lib

Yeah, also redux-act and probably redux-actions also does it. However, wondering whether this is still an issue? If someone uses custom redux actions in FSA style, would it be a problem to support both FSA and not FSA actions?

And, if you see any problems and we would still need to support both, wondering how we should do that. I don't wanna force FSA, but how to make it configurable? createQuery would need to have an option like useFSA, but passing it in all places would be terrible. Global config woud be possible, but then it would need to be a global object, which I don't like. It could be supported by:

  • a separate method like createQueryFSA, but I don't really like the longer name...
  • one could create own wrapper and pass useFSA oneself, like const createQueryCustom = (type, driverConfig, options) => createQuery(type, driverConfig, options, true), but then, one would need to create wrapper for mutations and... incoming subscriptions too
  • global setter like setFsa(true), but then the order of imports could wrapper, and with ES6 imports this can lead to unexpected bugs
  • or maybe we should support only FSA?

This is a really tough nut to crack

about variables - it's not always possible to know in ahead

very good example, I assumed that useQuery will rerender on variable change and new memoized load will be used, but indeed, it is not always the case, I will need to make this change!

@klis87
Copy link
Owner Author

klis87 commented Jan 24, 2021

Adding to this discussion, it is worth mentioning, that even currently all utility actions like resetRequests, abortRequests and so on are NOT FSA actions! Just request actions can by of FSA type, because for now there is no official helper to create them. Anyone could create them however one wants, either manually, or by an action creator library like redux-act. And this was precisely the reason why FSA request actions are supported. But once official helper like createQuery would be added, then creating request actions will become standardized, hence no need to support them in FSA form anymore. People could still create own Redux actions with any library like toolkit or redux-act, just actions from this library will be not FSA.

@klis87
Copy link
Owner Author

klis87 commented Jan 24, 2021

To sum up, the only way I could support FSA action is to transform all actions to FSA mode, for example resetRequests would be:

{ type: 'RESET_REQUESTS', payload: ['QUERY1', 'QUERY2'] }

but really I am not sure we should go FSA or not FSA. However, supporting both would just cause big overhead without too big reasons.

@avasuro
Copy link

avasuro commented Jan 25, 2021

@klis87

I read your previous answers, and there is something to think about, so I will answer later (hope I will not forget about them). But according to this answer:

To sum up, the only way I could support FSA action is to transform all actions to FSA mode, for example resetRequests would be...

I think there is no sense to transform internal actions of this library to FSA (or not FSA). When I wrote about that it is worth keeping the support of FSA events, I meant the ability to create actions for requests. Like this library should support both versions of actions:
{type: 'MY_ACTION', request: {url: '/getSomething'}}
and
{type: 'MY_ACTION', payload: { request: {url: '/getSomething'} }}

Because "initial" actions can be created in several ways (using plain object, or using some action creator library, or using some utility function provided by this library) and it is not always easy to produce only non-FSA (or only FSA) actions. While "internal" actions (reset, abort) can be created only via utility functions provided by this library (in theory they also can be created manually, but that is unsupported way, so no need to take it into account, I think).

@klis87
Copy link
Owner Author

klis87 commented Jan 25, 2021

@avasuro Good points. The question is then, will all people use createQuery, createMutation and createSubscription helpers? In my opinion yes, and in fact this way will be advocated in all examples and documentations. This is the only way I can show nice API in examples, without duplicating action and constant imports in places like useQuery.

But, if not, if some people preferred not using those helpers, then it would be enough to support fsa request actions in reducers and middleware, helpers themselves wouldn't need to support fsa flag.

@klis87
Copy link
Owner Author

klis87 commented Jan 26, 2021

There is also one important thing to put in the equasion, I am pretty sure that the next major version will support request actions only created from Redux action creator lib, be it redux-act or built-in createQuery. This is because I don't want people to pass both constant and action creator to useQuery (action prop will be removed). This even more increases the chance that people will create requests only via built-in helpers. Then, request actions also will become some kind of internal actions. Then, they can be FSA, or not, it doesn't really matter.

@avasuro
Copy link

avasuro commented Jan 27, 2021

Mmm, if I understand right: to simplify library code it will be good to choose some one type of actions (FSA or non-FSA) to use inside this library.

In short:

If you force users to use only your helpers (createQuery etc.) - then as you said it doesn't matter which type of action to use. But if you leave the opportunity for users to choose which way of creating events to choose, then only FSA actions needs to be supported.

Longer explanation:

Should we force users to use only our helpers to create actions?
I don't think that it is good practice (until it is absolutely unavoidable), it's better to leave ability for user to decide how to create actions.

How user can produce actions?
I see only 3 possible ways how end user may want to produce actions:

how action can be created produces FSA or non-FSA actions?
own helpers
(e.g. createQuery)
can be any
action creator lib
(e.g. redux-act)
can be any (in most cases only FSA)
manually can be any (the only restriction can be some internal development rules for specific project, which forces to use only FSA actions)

Pros and cons of choosing FSA/non-FSA

action type to use cons pros
only non-FSA - impossible to create such action using most action creator libs (they produce only FSA)
- this type may be prohibited for use by internal development rules for specific project
- ???
only FSA - not backwards compatible with current lib version (at least in all examples in documentation you are using non-FSA actions, so I think most of the users who manually create events will use this type of actions right now)
- will work in all cases: all action creator libs that produces non-FSA actions can produce FSA actions too, same thing for projects where mostly non-FSA actions used - nothing restrict such users to produce FSA actions for this library

So, according all above: I think it's better to use FSA actions in this lib, because they will work in all possible use cases. Yes, this approach is not backwards compatible, but because currently library supports both actions types dropping support of any of them is a breaking change, so this con is not so significant from my point of view.

@avasuro
Copy link

avasuro commented Jan 27, 2021

... I am pretty sure that the next major version will support request actions only created from Redux action creator lib, be it redux-act or built-in createQuery. This is because I don't want people to pass both constant and action creator to useQuery (action prop will be removed). ...

Fair enough, then you can exclude all things related to "manually created actions" from my previous answer.

@klis87
Copy link
Owner Author

klis87 commented Jan 27, 2021

@avasuro thx for your analysis, indeed it looks like going FSA all the way could be the best option. Actually we almost have FSA anyway, as we use meta convention. We could even change request into payload without nesting request in payload, because probably next version will recognize request actions and queries differently. Probably we will just have a new mandatory meta.requestType: 'QUERY' | 'MUTATION' | 'SUBSCRIPTION'

I know this is quite a big change, but this will be for a reason. Not for all drivers it is possible to deduct whether it is query or mutation. For example, for promise driver it is always necessary to pass meta.asQuery. This will also simplify many things, for example we could remove isRequestAction and isRequestActionQuery from handleRequests config, as adjusting this won't be necessary anymore - meta.requestType will tell us all explicitely about the type.

But with createQuery and family, meta.requestType will be added just automatically. I think we could also add meta.variables automatically, then we could use this info for example to make effect takeLeading instead of takeLatest for a query by default, because without variables it doesn't really make sense to abort previous pending query and start a new one, it would be a nice optimisation, done automatically for the user. But this requires obligatory usage of createQuery helpers (in theory one could do this manually, but why?). And frankly, I don't understand why someone would use redux-act etc as createQuery will do the very same thing, but a little more. But, perhaps FSA should be enforced, as then all will be compatible with createReducer of many libraries like redux-act, which require FSA structure.

Also, I have some more plans for action creator helpers - one of my TODO is adding support for offline mutations - they will be queued when offline, and could be auto dispatched when going back online. But callbacks are not serializable, so we cannot just save mutations to local storage for example and recreate them on reload when going online. But I am pretty sure I will be able to mitigate it with some alternatives, thx to nothing else than request creators. This is yet another argument to really enforce usage of this, but in the docs we should of course explain, what it does under the hood, so Redux users will understand what's going on behind the scenes

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