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

Revert permanentFilter in setFilter's useCallback [FIX] #4688

Conversation

fancyaction
Copy link
Contributor

This reverts #4508 which resulted in a bug where setFilter is re-rendered on every input, wiping out any user text. Alongside the recent #4650, this feature seems to work now and I can successfully filter a referenceInput based on another form's input. Please merge at your convenience. Thanks!

@fzaninotto
Copy link
Member

I can't reproduce the bug you're mentioning. Would you mind linking to a CodeSandbox showing the bug?

@fancyaction
Copy link
Contributor Author

fancyaction commented Apr 16, 2020

I'm having a hard time recreating in Codebox: https://codesandbox.io/s/fast-leftpad-onyt9?file=/src/posts/PostCreate.js

To debug I added a simple (in my local useFilterState.js)

useEffect(function () {
        console.log('permanentFilter HAS CHANGED!!!', permanentFilter);
    }, [permanentFilter]);

I see this fire 12 times if I just type "s" into the AutocompleteInput. Furthermore, I see this fixed if I replace permanentFilter with permanentFilterProp.current in useCallback.

I'm struggling to see why this is unique to my local and I find it hard to trust the Codebox example because it doesn't fuzzySearch or render a list of choices like it should -- wondering if it works there because of the way ra-data-fakerest works? Would be good to know if this is unique to my local -- not sure how to accomplish that other than others confirming these features work for them in their codebase.

Any advice is welcome. Spending a lot of time trying to recreate error in sandbox and debug my local.

For context here is my code (I disabled all other form inputs and hardcoded the filter to confirm this is still not working -- works fine until I enable filter prop):

                <ReferenceInput
                    label="Well Stage"
                    source="well_stage_id"
                    reference="well-stages"
                    filter={{ well_id: 50 }}
                    filterToQuery={(searchText: string) => ({ description: `%${searchText}%` })}
                    sort={{ field: 'description', order: 'ASC' }}
                    allowEmpty
                    perPage={100}
                    validate={[required()]}
                    {...rest}
                >
                    <AutocompleteInput shouldRenderSuggestions={(val: string) => 2 < val.trim().length} />
                </ReferenceInput>

@fzaninotto
Copy link
Member

my 2c: The permanent filter you're adding should be extracted, because it's currently a new object at every render. That may cause the bug.

// outside of the component function
+const permanentFilter = { well_id: 50 };

...

                <ReferenceInput
                    label="Well Stage"
                    source="well_stage_id"
                    reference="well-stages"
-                   filter={{ well_id: 50 }}
+                   filter={permanentFilter}
                    filterToQuery={(searchText: string) => ({ description: `%${searchText}%` })}
                    sort={{ field: 'description', order: 'ASC' }}
                    allowEmpty
                    perPage={100}
                    validate={[required()]}
                    {...rest}
                >
                    <AutocompleteInput shouldRenderSuggestions={(val: string) => 2 < val.trim().length} />
                </ReferenceInput>

@fancyaction
Copy link
Contributor Author

fancyaction commented Apr 30, 2020

Thank you very much for that tip! It seems putting that prop into a variable and wrapping it with a useMemo to prevent constant renders did the trick.
const wellStageFilter = useMemo(() => ({ well_id: currentWell }), [currentWell]);

This is a very cumbersome solution, however, that will require a lot of minor changes throughout our app. Before I go ahead, can you confirm this is the proper solution? If so, this should be noted in the docs. As a user, I assume I pass a filter and that shouldn't cause rerenders unless the filter itself should change.

I've been having a lot of trouble with filters lately. I've been stuck on RA v.3.3.0. I'm wondering if this is partly due to using WiSXL's fork of react-admin-date-inputs (the master branch is pretty dead) and that's causing dependency conflicts? I noticed he just pushed this #4730 which may be a fix I need? Hope to find out with the next RA release!

As always, any advice is appreciated. You're doing great work!

@fancyaction
Copy link
Contributor Author

I came up with my own reusable solution so I don't need to use useMemo all over the codebase for filter objects. Do you think this could be incorporated into the ReferenceInput component? You already are doing something similar in its useEffect but the issue is that the setFilter fn updates every render because, like you said, the filter object is new every render w/o lodash's isEqual helper.

import React, { useMemo, useState, useEffect } from 'react';
import { ReferenceInput } from 'react-admin';
import lodash from 'lodash';

const MemoizedReferenceInput = ({ filterToQuery, filter, children, ...props }) => {
    const [memofilter, setMemoFilter] = useState(filter);
    const memoizedFilterToQuery = useMemo(() => filterToQuery, []);

    useEffect(() => {
        if (!lodash.isEqual(memofilter, filter)) {
            setMemoFilter(filter);
        }
    }, [filter]);

    return (
        <ReferenceInput filter={memofilter} filterToQuery={memoizedFilterToQuery} {...props}>
            {children}
        </ReferenceInput>
    );
};

@fzaninotto
Copy link
Member

I found a better solution in #4784, so I'm closing this PR.

@fzaninotto fzaninotto closed this May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants