-
Notifications
You must be signed in to change notification settings - Fork 790
Suggest feature: Debounce option in query operation. #450
Comments
Hey @zxlwbi199101 and @ksmth. Thanks for the issue and PR! Here are my thoughts: Even if it would be a little bit nicer to use if it's integrated with react-apollo, it's something that I think should clearly be kept outside because it can be done by simply wrapping the component. We have to carefully consider every addition to Apollo, because it isn't as simple as just clicking 'merge'. When we merge something, we have to make sure that it keeps working, even as we make more changes to Apollo Client. The more features we have, the harder it becomes to maintain the core functionality and develop it further, so whenever something can be reasonably done outside of Apollo, that feature should not be added in my opinion. |
@helfer please explain how it can be done by wrapping the component as I have trouble understanding how I can both debounce a value change down the hierarchy while at the same time passing the value for the component to consume. |
@ksmth I'm no React expert, but I think something like this should work: You keep a state variable in the wrapper component to decide whether or not to re-render the child. When you render the child you set a timeout. Until that timeout elapses you don't re-render the child, even if the props are updated. When the timeout elapses you re-render the child component. Does that make sense? Another maybe more suitable approach would be to debounce the event handler at the source so your props don't change that frequently. |
It's not as straight forward as it first seems. Consider this example:
These two requirements are in conflict, as you want the most up to date data for your Now if you delay either the rendering or the dispatch of the
You'd then get something like:
As I imagine this to be a really common use case for Apollo though, I'd still argue for it to be a feature in In order to make this a useful addition to the docs, I'd like to know where to put it. Would it be another menu item in the |
@ksmth I agree that you want your input field to update immediately, but your query shouldn't run immediately, but why not just make the list / result component be either a sibling or a child to the input component? That way the input can rerender independently from the list, which will re-render when the query is fetched again. Debouncing is certainly a useful feature, but if you ask me we already have features in React Apollo that don't really need to be there. The same is true for Apollo Client core, which currently has some features that should live either in React Apollo or in an integration layer. Having these features isn't bad per se, but unfortunately every feature that gets added makes the library harder to maintain because it adds code that could interact with other features and cause the client to break in unexpected ways. The larger the library, the harder it will be and the longer it will take to get each little bug fixed, so I think it's not just a benefit for maintainers to keep things modular, but also a great benefit for anyone who is using the library, because it means they don't need to wait for the library to be updated to fix bugs. |
When using Redux (or any other flux pattern) together with controlled inputs – siblings or not – you effectively need to create virtual debounced prop only for the purpose of passing to apollo. As far as making the typical redux app's state management clean and not concerned with data fetching concerns (which, maybe a bit contrived, can be compared with timeout handling, retries etc), support in the client would be great. If the client is to be kept simple which I'm all in favor of, the available options are to either say that the debounced values should be part of the global state because, maybe, some other part of the app should render differently depending on the debounced value. Or wrap the graphql-components with hidden IMHO, it feels a little awkward to add such a HoC for the purpose of injecting the debounced prop, read it during graphql config and then strip it away when mapping props to the underlying component. It also adds complexity in making sure to know the name and use the debounced props in the query options. I have a similar version of the HoC above but instead of passing a list of prop names I do: export function withDebouncedProps({ debounce, props }: {
debounce: number,
props: (props: Object) => Object,
}) {
...
} which allows me to control the names of the debounced props and somewhat easier type annotations. Maybe targeting the wrapper directly for the |
Making the List / Result component a child or sibling to the input component on its own wouldn't solve the problem, you'd still have to debounce the value changes in some way. It doesn't matter where the results component lives, as long as it depends on a state that is frequently changing, such as the value of an input component, or any other input for that matter, it needs to be debounced as you'd be potentially sending too many requests. As I said previously, I completely understand and agree with the importance of keeping the library as small as possible. We only disagree on whether debouncing is a common use case or not. It seems to me, running queries based on changing values is particularly common, but I'm also aware that my own needs might conflate my perception of the importance of this feature. What I think we both can agree on though is, that giving users of |
Adding to my idea above, to create a wrapped I'll see if I can find some time to play around with that concept. |
I have encountered a similar use-case for debouncing, in addition to the search mentioned here as well. In my case, I have a username input that also does on-the-fly validation to check if the username has already been taken or not. I also think that debouncing can be a very general use-case for anything to deal with the network - especially to reduce the number of requests. In apollo-client there is also a batchedNetworkLayer that works on the same principle. However for a client-side prop that does not need such an over-engineered solution, using a wrapped |
This would be a great recipe and/or an external package. I don't think it should be in the core RA though! |
FWIW, here is a quick solution I came up with using recompose. I only needed to debounce a single prop, but this could be extended to more I'm sure. import _debounce from 'lodash/debounce'
import _omit from 'lodash/fp/omit'
import { compose, withState, withHandlers, mapProps, withProps } from 'recompose'
const omitProps = compose(mapProps, _omit)
const debounceProp = (propToDebounce, delay = 150, maxWait = 500) => {
const propAccessor = `_debouncedProp_${propToDebounce}`
const propSetter = `_setDebouncedProp_${propToDebounce}`
const propDebouncer = `_setPropDebounced_${propToDebounce}`
const debounceState = withState(propAccessor, propSetter, props => props[propToDebounce])
const debounceHandler = withHandlers({
[propDebouncer]: props => _debounce(props[propSetter], delay, { maxWait })
})
const finalPropMap = withProps(props => {
if (props[propToDebounce] !== props[propAccessor]) {
props[propDebouncer](props[propToDebounce])
}
return {
[propToDebounce]: props[propAccessor]
}
})
return compose(
debounceState,
debounceHandler,
finalPropMap,
omitProps([propAccessor, propSetter, propDebouncer])
)
}
export default debounceProp And I'm using it like this (my use case is a redux-controlled search input driving the const enhance = compose(
connect(mapStateToProps),
debounceProp('searchQuery', 500, 1500),
withFeed,
branch(({ data }) => data.loading, renderComponent(LoadingView)),
branch(({ data }) => data.error, renderComponent(ErrorView)),
) |
@ksmth : thanks for your solution, works great! (why didn't you use PureComponent instead of shallowEqual?) I agree with @ksmth that debouncing queries is something that should be supported and I agree with @helfer not to overcomplicate the library, so that makes me a bit crazy.
|
Just for reference, I created a debouncing hoc for input components. It switches to localstate value while debouncing and switches back afterwards, so this can be used with redux.
Usage as hoc:
Component:
|
Just wanted to throw out my solution that is considerably less code. Any downside to this approach over debounceProp? import { debounce } from 'lodash'
// Utilizing closures to debounce functions from props.
// If debounce is applied within the component, the "debounced function"
// will change each time. By defining it statically here, react will always
// consider this to be the same function.
const debounceCall = debounce(func => func(), 250);
const SearchComponent = ({
// These methods/state can be managed by redux
// or something else by a parent component
inputValue,
updateInputValue,
searchValue,
updateSearchValue,
data // <-- data from apollo
}) => {
return (
<div>
<div>Searching for {searchValue}</div>
<input
type="text"
value={inputValue}
onChange={e => {
updateInputValue(e.target.value);
debounceCall(() => {
updateSearchValue(e.target.value);
});
}}
/>
</div>
);
};
const withData = graphql(SEARCH_QUERY, {
options: props => ({
variables: { search: props.searchValue },
}),
});
export default withData(SearchComponent); |
Yet another solution: https://github.com/saltycrane/react-debounced-props Higher order component: const withDebouncedProps = (propNames, debounce) => BaseComponent =>
class ComponentWithDebouncedProps extends React.Component {
static displayName = `withDebouncedProps(${BaseComponent.displayName ||
BaseComponent.name})`;
constructor(props) {
super(props);
this.state = pick(props, propNames);
}
componentWillReceiveProps(nextProps) {
this.debouncedUpdate(nextProps);
}
componentWillUnmount() {
this.debouncedUpdate.cancel();
}
debouncedUpdate = debounce(nextProps => {
this.setState(pick(nextProps, propNames));
});
render() {
return <BaseComponent {...omit(this.props, propNames)} {...this.state} />;
}
}; Example usage: import { debounce } from "lodash";
import { compose, graphql } from "react-apollo";
const MyEnhancedComp = compose(
withDebouncedProps(["regionName"], func => debounce(func, 200)),
graphql(REGIONS_QUERY),
)(MyComponent); |
Edit: Scratch my last message; the solution @saltycrane posted works perfectly. I've added a Typescript version here |
Seeing that various people are coming up with different and complex solutions makes a stronger argument that this should be provided by apollo. |
Sorry to be late to the party but another argument in favor of adding debouncing support as part of Apollo core as opposed to on handler event is that such debouncing could be cache-aware and not apply to already cached values. For example, using the use case of instant search, if I type "foo", it makes sense to debounce, say after 500ms, to only send a query with I understand both sides of the argument, but I think having a |
I think this is such a common need in any react app, that it should be a part of react-apollo. |
How do you debounce with the hooks? |
@EloB Untested, but something along these lines would do the trick: const useData = (input: string) => {
const [debounceValue, setDebouncedValue] = useState(input);
useEffect(() => {
const handler = setTimeout(() => {
setDebouncedValue(input);
}, 500);
return () => {
clearTimeout(handler);
};
}, [input]);
const response = useQuery(QUERY, {
variables: {
input: debounceValue,
},
});
return response;
}; |
I’m thankful for your deed but my thought was more about I need 10-11 extra lines of code for something that is such common need. |
usage scenario
I use redux, and wrap MyListComponent by
graphql(queryStr, { options })
, when some options changes, it fetch from the server immediately. Have a look at 2 use scenarios:list?filter=name&sort=age&...
, the params are parsed in componentWillmount then dispatched to redux store. So when first loaded, the graphWrapper made a fetch with init options, then made another fetch with the dispatched values. So the first one is a waste.I understand for both scenario is not a apollo specific problem, and can be fixed easily through _.debounce or wrap another component (maybe a little bit harder to debounce when binding the redux store value to that input). But I think the code will be more clean if apollo support a debounce option.
what I have tried
Apparently this just debounce queries with their name, and if there are more than one queries to the same api, this middleware will cause bug. And if added a whitelist or blacklist to operationName, I don't think it's a graceful design.
thoughts
The text was updated successfully, but these errors were encountered: