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

Support 0 as snackbar key #509

Closed
mrodz opened this issue Aug 16, 2022 · 2 comments
Closed

Support 0 as snackbar key #509

mrodz opened this issue Aug 16, 2022 · 2 comments

Comments

@mrodz
Copy link

mrodz commented Aug 16, 2022

Function in question:

export const isDefined = (value: string | null | undefined | number): boolean => (!!value || value === 0);

Current Behavior

As you can see, this function uses a double shabang (!!) operator to test whether a value is defined or not. However, one should not rely solely on type coercion here—it will mistake zero and empty strings for undefined values.

Context

Unless this behavior is intended—in which case it should be expressly stated in the documentation—this function should be re-worked. I came across this issue when I tried to count Snackbars as they constructed.

/*
SIMPLIFIED
*/

const [count, setCount] = useState(0);
const { enqueueSnackbar, closeSnackbar } = useSnackbar();

const myCallback = () => {
    enqueueSnackbar('I love snacks', {
        persist: true,
        key: count,
        action: () => (
            <Button onClick={() => closeSnackbar(count)}>X</Button>
        )
    });
    setCount(count + 1)
};

As you can see, when attempting to set zero as a key, the program crashes and outputs the following stack trace:

SnackbarProvider.tsx:181 Uncaught Error: handleEnteredSnack Cannot be called with undefined key
    at SnackbarProvider._this.handleEnteredSnack (SnackbarProvider.tsx:181:1)
    at chainedFunction (createChainedFunction.js:21:1)
    at chainedFunction (createChainedFunction.js:21:1)
    at chainedFunction (createChainedFunction.js:21:1)
    at chainedFunction (createChainedFunction.js:20:1)
    at Object.onEntered (Slide.js:131:1)
    at Transition.js:281:1
    at Transition.nextCallback (Transition.js:343:1)
    at callCallback (react-dom.development.js:13923:1)
    at commitUpdateQueue (react-dom.development.js:13944:1)

Why it matters

Zero as an index/key makes sense to most developers. The obscurity of this stack trace doesn't help resolve any confusion that might arise from this, either, which could lead to many minutes of puzzled head-scratching.

@iamhosseindhv
Copy link
Owner

I don't think isDefined is the issue since it explicitly checks for value === 0 and counts 0 as a valid value. The intention has been to support 0 as snackbar key (see #187). I think this line has causes a regression:

if (extraArg && argums.indexOf(extraArg) === -1) {
as it is not accounting for extraArg being zero.

@iamhosseindhv iamhosseindhv changed the title Re-think how isDefined() function is implemented Support 0 as snackbar key Oct 16, 2022
@iamhosseindhv
Copy link
Owner

Closing due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants