reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.71k stars 1.17k forks source link

How to import the QueryHooks interface? #4390

Closed rwilliams3088 closed 5 months ago

rwilliams3088 commented 5 months ago

I wrap some of my endpoints in a generic class that is able to integrate RTK Query endpoints with AgGrid as a ServerSideDatasource. One of the interfaces that I rely upon is the QueryHooks interface, as I make use of the useLazyQuery() function.

I used to be able to import this from @reduxjs/toolkit/dist/query/react/buildHooks, but that no longer works now that I've upgraded to the latest version of @reduxjs/toolkit. Nor does @reduxjs/toolkit/query/react or @reduxjs/toolkit/query/react/buildHooks.

I can work around this for now by duplicating the interface locally... but not the ideal solution. Please advise on how to properly import this interface into my code.

EskiMojo14 commented 5 months ago

could you show how you're using the QueryHooks interface specifically?

rwilliams3088 commented 5 months ago

Sure. Here are the props to my wrapper class, which uses the interface as part of the endpoint type definition.

export interface AgGridDatasourceProps<
  OPTS extends AgGridQueryArgs_Options = AgGridQueryArgs_Options,
  QA extends AgGridQueryArgs<OPTS> = AgGridQueryArgs<OPTS>, 
  RT extends AgQueryResponse = AgQueryResponse, 
  QD extends QueryDefinition<QA, any, any, RT, any> = QueryDefinition<QA, any, any, RT, any>
  > {
    endpoint: ApiEndpointQuery<QD, any> & QueryHooks<QD>;
    options?: Omit<OPTS, 'countOnly'>;
    queryArgs?: (orig: AgGridQueryArgs) => QA,
  }

And this is used in the class to acquire a trigger function to invoke the query dynamically:

export function useAgGridDatasource<
  OPTS extends AgGridQueryArgs_Options = AgGridQueryArgs_Options,
  QA extends AgGridQueryArgs<OPTS> = AgGridQueryArgs<OPTS>,
  RT extends AgQueryResponse = AgQueryResponse,
  QD extends QueryDefinition<QA, any, any, RT, any> = QueryDefinition<QA, any, any, RT, any>, 
  P extends AgGridDatasourceProps<OPTS, QA, RT, QD> = AgGridDatasourceProps<OPTS, QA, RT, QD>
  >(props: P): AgGridServerSideDatasource {

  const { endpoint, options, queryArgs } = props;
  const [trigger] = endpoint.useLazyQuery();
  ....
  const query = trigger(qa, false); // invoked in a callback function by AgGrid
  ....

}

Not having access to this interface breaks my entire application

EskiMojo14 commented 5 months ago

is useLazyQuery the only thing you're using endpoint for?

export interface AgGridDatasourceProps<
  OPTS extends AgGridQueryArgs_Options = AgGridQueryArgs_Options,
  QA extends AgGridQueryArgs<OPTS> = AgGridQueryArgs<OPTS>, 
  RT extends AgQueryResponse = AgQueryResponse
  > {
    endpoint: { useLazyQuery: TypedUseLazyQuery<RT, QA, any> }
    options?: Omit<OPTS, 'countOnly'>;
    queryArgs?: (orig: AgGridQueryArgs) => QA,
  }

export function useAgGridDatasource<
  OPTS extends AgGridQueryArgs_Options = AgGridQueryArgs_Options,
  QA extends AgGridQueryArgs<OPTS> = AgGridQueryArgs<OPTS>,
  RT extends AgQueryResponse = AgQueryResponse,
  >(props: AgGridDatasourceProps<OPTS, QA, RT>): AgGridServerSideDatasource {

  const { endpoint, options, queryArgs } = props;
  const [trigger] = endpoint.useLazyQuery();
  ....
  const query = trigger(qa, false); // invoked in a callback function by AgGrid
  ....

}
rwilliams3088 commented 5 months ago

is useLazyQuery the only thing you're using endpoint for?

That plus the type constraints for the query args and such

rwilliams3088 commented 5 months ago

Found a work-around for the moment (but the hook interfaces still need to be exposed properly - please and thank you). Import it via the local file system in the node_modules folder:

import { QueryHooks } from "../../../../node_modules/@reduxjs/toolkit/dist/query/react/buildHooks";
EskiMojo14 commented 5 months ago

did you try the code I posted?

rwilliams3088 commented 5 months ago

did you try the code I posted?

I missed the change you made above; thought you were just quoting my code haha. Plugging that in works. I'd say the QueryHooks interface should still be exposed as being able to programmatically access the standard interfaces is important and I'll be keeping my import above for now. I don't like the idea of constructing my own interfaces like this to wrap standard functionality - the QueryHooks interface should simply be exposed for developers. It isn't an arbitrary implementation detail - those hooks are the primary means through which react developers use the library. It creates unnecessary problems trying to hide it.

EskiMojo14 commented 5 months ago

i don't agree, sorry - I think it's actually very rare for users to be in a situation where they need the whole QueryHooks type, and it's also built in a way that's not particularly user friendly, since it needs the whole QueryDefinition shape.

anything we consider to be useful to users, we do export - and as my code shows, we have specific Typed utilities for most of the hook related types, which are designed to be more user friendly.

it's also good practice to design your types to only require the things the function uses, as it makes your function more flexible and testing easier.

rwilliams3088 commented 5 months ago

It doesn't making testing easier to arbitrarily limit type information; you simply test what you use. Rather you make your code more brittle by not using standard interfaces and instead creating an amalgamation of properties. It also makes the code far less readable.

Also, the hooks are already exposed to end users - that's how they interact with the library in react. You are just creating obstacles to conveniently access the interface that you are already exposing through the API slices. But suppose we'll have to agree to disagree. This library is still immature; I'm sure its just a matter of time before others are requesting access to such interfaces as well for more advanced use cases.

EskiMojo14 commented 5 months ago

RTK Query was released nearly 3 years ago.

rwilliams3088 commented 5 months ago

Yes, and? It's barely on v2. Whilst I enjoy the framework, it still has much room for improvement. For instance: there is no built-in support for handling the serialization and de-serialization of data to/from the endpoints. I had to build a custom solution for that.

phryneas commented 5 months ago

there is no built-in support for handling the serialization and de-serialization of data to/from the endpoints

That's query and transformResponse or another baseQuery.

I agree with Ben above: QueryHooks is an internal type that's not meant to be used by users. As he said, the type arguments you'd have to manually pass into it for that are monumental. It's meant to be inferred. For manual typing, we export specific helper types.

It's barely on v2.

We increase the major for breaking changes only, as it is intended by SemVer. All that v2 means is that we have avoided breaking changes pretty successfully and had to do breaking changes only once. You have to count minors for features and patches for bugfixes. Keep in mind that we don't release often, so every minor will contain lots of features.

markerikson commented 5 months ago

@rwilliams3088 : I strongly disagree with it's "immature" and "barely on v2".

Redux Toolkit has been out since October 2019, and RTK Query since 2021. That "v2" is a sign of stability, both of RTK Query specifically and Redux Toolkit as a whole. We've added numerous new features to RTK since its inception, and those additions have been non-breaking. We have millions of downloads a month, and RTK is used in some of the largest apps in the world. Those are signs of stability. High version numbers are not indicative of "maturity".

For instance: there is no built-in support for handling the serialization and de-serialization of data to/from the endpoints

I don't know what you mean by this. RTKQ does JSON transforms automatically. What additional "serialization" do you need for your use case?

We're always interested in ways to improve RTK/RTKQ, so if you do have suggestions, please file issues with your requests and use cases. (But saying it's "immature" and complaining "it doesn't have this one thing I need" aren't very helpful, especially in a thread like this.)

rwilliams3088 commented 5 months ago

there is no built-in support for handling the serialization and de-serialization of data to/from the endpoints

That's query and transformResponse or another baseQuery.

Yes, there is sufficient support to add your own serialization / deserialization logic - but its entirely custom, not a standardized feature of framework. And there remain issues with this fact - such as handling Dates and other complex objects as inputs to an endpoint. It can be done - but it requires a good bit of tinkering to do (which I don't think is documented either).

Again - love the framework, I moved my company over to it off of GraphQL. But let's not pretend the RTK Query is a mature framework with all the bells and whistles.

I agree with Ben above: QueryHooks is an internal type that's not meant to be used by users. As he said, the type arguments you'd have to manually pass into it for that are monumental. It's meant to be inferred. For manual typing, we export specific helper types.

There are a few generic arguments, but nothing so complicated that you can't use it. In fact, the the "helper" types require basically all the same type parameters. So the argument that passing in those same arguments to the QueryHooks interface is a monumental task falls a bit flat.

It's barely on v2.

We increase the major for breaking changes only, as it is intended by SemVer. All that v2 means is that we have avoided breaking changes pretty successfully and had to do breaking changes only once. You have to count minors for features and patches for bugfixes. Keep in mind that we don't release often, so every minor will contain lots of features.

Yes - you've all done a great job maintaining the project and of querying the community about feature enhancements we'd like to see. But, again, let's not pretend the framework is a mature one with all the bells and whistles, or even that all the major bugs have been fixed. Last I checked I still can't use tags for caching in combination with Lazy queries properly. And since my application primarily uses Lazy queries, that means I can't make use of the frameworks caching mechanism. :/ RTK Query still has a long ways to go before it can claim to be a mature development framework

rwilliams3088 commented 5 months ago

@rwilliams3088 : I strongly disagree with it's "immature" and "barely on v2".

Redux Toolkit has been out since October 2019, and RTK Query since 2021. That "v2" is a sign of stability, both of RTK Query specifically and Redux Toolkit as a whole. We've added numerous new features to RTK since its inception, and those additions have been non-breaking. We have millions of downloads a month, and RTK is used in some of the largest apps in the world. Those are signs of stability. High version numbers are not indicative of "maturity".

For instance: there is no built-in support for handling the serialization and de-serialization of data to/from the endpoints

I don't know what you mean by this. RTKQ does JSON transforms automatically. What additional "serialization" do you need for your use case?

We're always interested in ways to improve RTK/RTKQ, so if you do have suggestions, please file issues with your requests and use cases. (But saying it's "immature" and complaining "it doesn't have this one thing I need" aren't very helpful, especially in a thread like this.)

You shouldn't take the critique that it's not a mature framework negatively; that's not my intention. Like I said, I moved my company to this framework off of GraphQL (which I hate using) - and I'm glad I did so even if the framework is still not where I'd like it to be. It's a project I'd like to support and see grow.

That said, it's a fact that a lot of basic functionality that I would like to see as a developer isn't there yet (and hiding interfaces that were previously public isn't helpful). It's not one or two features missing - I've listed several in this thread, and they are ones I've brought up previously in other threads.

As for serialization and deserialization - I refer to complex objects like dates and custom classes

phryneas commented 5 months ago

and hiding interfaces that were previously public isn't helpful

The dist folder was never considered public. The only public entry points were the officially documented ones, @reduxjs/toolkit, @reduxjs/toolkit/query, @reduxjs/toolkit/query/react and now @reduxjs/toolkit/react.

The type you are referring to was (like everything in the dist folder) always an implementation detail and never exported for public use.

In fact, the the "helper" types require basically all the same type parameters.

Required by QueryHooks: a QueryDefinition, which requires:

Required by e.g. TypedUseQuery

that's a monster object combining five generics against straight up three generics.

Last I checked I still can't use tags for caching in combination with Lazy queries properly.

If you find a bug, please report it or file a PR fixing it. Not reporting and calling things immature is not helpful.

rwilliams3088 commented 5 months ago

If you find a bug, please report it or file a PR fixing it. Not reporting and calling things immature is not helpful.

I feel you are taking the comment that the framework is "immature" as an insult, and I just want to re-iterate that it is not intended to be derogatory. All frameworks require time to mature into their own - and this one is no different. It still has a long ways to go, as noted by the several basic issues I pointed out in this thread.

And, for the record, they are issues that I have reported and discussed previously in other threads as I found them while integrating RTK Query with my projects. If these issues don't sound familiar to you, then perhaps its time I dig up and revive those issues or re-file them.

phryneas commented 5 months ago

Please do so. I have no recollection of anything regarding invalidation and lazyQueries, and it frankly seems very weird, since those two concepts barely have any touching points.

rwilliams3088 commented 5 months ago

A thread on passing complex objects in/out of RTK Query: https://github.com/reduxjs/redux-toolkit/issues/3640

A thread on Lazy Queries and Tag Invalidation: https://github.com/reduxjs/redux-toolkit/issues/2802