nkbt / react-debounce-input

React component that renders Input with debounced onChange
MIT License
450 stars 61 forks source link

Validation support #130

Open dmatora opened 3 years ago

dmatora commented 3 years ago

On regular input onChange I can do

        if (isNaN(value) || value < 0) {
            return;
        }

and it will not allow typing in letters or negative value but on DebounceInput it will allow typing anything (while still preventing getting letters or negative into controlled var value) which basically breaks component

nkbt commented 3 years ago

Can you provide a working example please as without context I struggle to understand why this might happen. Thank you 🙏

andybenedict commented 2 years ago

I have another situation that results in this rendering error occurring.

It is happening because of this check.

In short, the component only re-renders if a value is different than the previous value and different from the new state value. In @dmatora's case, he's disallowing negative or non-numbers, so when the value prop is not changed, it skips the re-render, but the entered value continues to display.

I'm getting this error when splitting off the decimal portion of a number to place in a companion dropdown (some users enter this value manually, so fractions are preferable to them, but other users are getting the data typed as decimals from a laser measurement tool). So I have a value of 21 and change it to 21.5, my script takes the floor and the mod 1 (21 and 0.5) and tries to update this component with the value 21 and a select box that displays .5 as 1/2, but because the previous value was 21, the component does not re-render, so the box continues to display 21.5

This could be corrected by changing the line to if (typeof value !== 'undefined' && (oldValue !== value || stateValue !== value)) { but it would largely negate the oldValue !== value check. The presence of a double check there suggests that it is dealing with some other edge case, but I don't know what it's there to handle, so I defer to your judgement rather than just submitting a pull request with that fix.

I couldn't glean from the commit what it was added for, but if it won't interfere with anything, I'd be happy to submit the PR.

andybenedict commented 2 years ago

I think this is the most stripped down component that results in my exact problem.

import React, { FunctionComponent} from 'react';
import { DebounceInput } from 'react-debounce-input';

interface InputElement {
    data: {
        [index: string]: string | number | string[] | null;
    };
    source: string;
    callback: (key: string, value: any) => void;
}

const InputField: FunctionComponent<InputElement> = ({
    data,
    source,
    callback,
}: InputElement) => {
    const min = 20
    const max = 100

    const sanitize = value => {
        value = Number(value);

        if (min > value) {
            return Number(min);
        }

        if (max < value) {
            return Number(max);
        }
    }

    const handleOnChange = e => {
        const newValue = e.target.value !== ''
            ? sanitize(e.target.value)
            : null;
        return (source && callback(source, newValue));
    }

    return (
        <DebounceInput
            debounceTimeout={250}
            type={'number'}
            value={data?.[source] === null ? '' : data?.[source]?.toString()}
            onChange={handleOnChange}
        />
    );
};

export default InputField;

Call this passing in these props from a parent element:

const [data, setData] = useState({});

const source = 'myValue';

const callback = (key: string, value: any, label: string) => {
    setData((state) => ({ ...state, [key]: value }));
};

If you put in a number less than 20, it replaces the number with 20 and uses the callback to update the parent, then the component updates on re-render. Likewise if you go over 100, it replaces it with 100.

The issue arrives when I subsequently enter another number less than 20 or over 100... so for example, enter 101, which gets replaced by 100 then enter a couple more 0's 10000. The data object on the parent gets updated, but the re-render does not occur because the value prop has not changed, even though the internal state has changed and is now displaying incorrectly.

nkbt commented 2 years ago

@andybenedict please provide a working example in codepen. Use one from readme as an example: https://codepen.io/nkbt/pen/VvmzLQ?editors=0010

So far from reading the code and compiling it in my head - I cannot see the problem and believe it all comes from the the way how app behaves.

andybenedict commented 2 years ago

Sure thing, this concisely demonstrates the problem: https://codepen.io/AndyBenedict/pen/zYWGKwe?editors=0010

I've given it a min of 10 and a max of 100... so enter any number between 10 and 100 and it will work normally. Any two consecutive numbers < 10 or > 100.

Try this sequence of numbers: 10 //works 20 //works 42 //works 8 //works, both state and component update to 10 7 //breaks, state updates to 10, but the component continues to say 7 110 //works, both state and component update to 100 1001 // breaks, state updates to 100, but the component continues to read 1001

I've provided a traditional input, you'll need to paste in the numbers because without the debouncing it's nearly impossible to type in a number.

andybenedict commented 2 years ago

The callback is being executed. The output of the debounce, and its internal state, is changing, I've added a console log to the codepen to confirm.

Prior to v3.2.1, the check to compare the new value to the previous value did not exist. And if I load it up at v3.2.0, it works as expected. I unfortunately don't know enough about class based react to fully understand the commit.

Can you elaborate on what that was added to do? Previously if the new value contradicted its internal state it would re-render. Is the second check just an optimization? Or was there a bug or specific desired behavior that's necessary to accomplish. I can certainly supply a PR to remove the extra check, but I'm hesitant to do so without knowing why it was added. It's the classic adage of not tearing down a fence until you understand why it was put up.

nkbt commented 2 years ago

I don't know much more because it was written like 3 years ago... I think it was just an optimisation to avoid unnecessary re-renders. As long as existing examples work - it's all good.

Meryovi commented 10 months ago

Is there any known workaround for this issue? I'm having the same issue described by @andybenedict