-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
False positive with no-unused-state #1910
Comments
You should never pass the props or state objects directly. The way to satisfy the warning is to extract the state fields you want, and make a new object with them. If you want to avoid rerenders, don’t pass an object - pass flat props. |
Interesting, how would you pass flat props to the React context provider given it only accepts the value prop? https://reactjs.org/docs/context.html#provider (Is this something that perhaps could be an exception to the rule, or apologies if I'm missing something?) Thanks |
I'd either use multiple context providers, or i'd make a new object, but memoize it so that the |
Thanks for the guidance - for me it doesn't make sense to use multiple context providers for things that are related to the same context (in fact I already am using multiple providers). Also Memoizing a new object doesn't really sit right with me just to satisfy this warning. I think I'll just disable the rule in my file for this case. Thanks for your help though, appreciate your quick responses 😃 |
What's your reasoning behind this? Passing import {ThemeContext, themes} from './theme-context';
import ThemeTogglerButton from './theme-toggler-button';
class App extends React.Component {
constructor(props) {
super(props);
this.toggleTheme = () => {
this.setState(state => ({
theme:
state.theme === themes.dark
? themes.light
: themes.dark,
}));
};
// State also contains the updater function so it will
// be passed down into the context provider
this.state = {
theme: themes.light,
toggleTheme: this.toggleTheme,
};
}
render() {
// The entire state is passed to the provider
return (
<ThemeContext.Provider value={this.state}>
<Content />
</ThemeContext.Provider>
);
}
} |
The react docs have many examples that have ended up being anti patterns, including putting arrow functions in class fields. Specifically, exposing a mutable object, for which mutations cause rendering behavior to be undefined, is an unnecessary risk and an unwanted expansion of your component’s public api.. Additionally, destructuring all state and props at the top of every function ensures that, like arguments, the inputs to the function are clearly marked at the top of the scope, ensuring enhanced readability and maintainability. |
come on this is bad advice, extracting and constructing a new context object in render is the anti-pattern and causes unnecessary context updates. Forcibly making all context single scalar values is a weird way around this that offers limited improvement while deepening the component tree unnecessarily. Moving the context value to a single state property artificially isn't objectively a better option essentially when the component is only a provider, it's state is the value. Keeping the rule the way it is is certainly the prerogative of the project and maintainers but claiming that the react docs are actually doing it wrong, and your opinions about readability and what constitutes maintainability is FUD. |
@jquense I'm not sure you're understanding the thread - while I do see how passing a new object created every time in There's nothing wrong with anyone claiming anyone is doing it wrong - that they're "the React docs" don't mean they're always doing things the best way. |
There is a security vulnerability in 'deep-extend', which is used by 'kcd-scripts'. This commit upgrades 'kcd-scripts' as well as all other dependencies to their latest verion. It also ignores the 'react/no-unused-state' rule due to an issue with react context (jsx-eslint/eslint-plugin-react#1910)
A discussion on best practices doesn't really add anything to the original concern though, and that's that @viralganatra is seeing a false positive whereby he's using all the state, but the rule is telling him he isn't. As @jquense said though, it's up to you folks to write the rules and such, but I think the discussion derailed pretty quickly. |
@callmenick indeed, but the limits of static analysis are that passing around the entire state, props, or context object makes it impossible to detect which properties are used. Since this is a bad practice anyways, though, there's not really much value in trying to address it. |
@ljharb In certain scenarios passing the entire state is necessary. Consider the following case:
So in this case if ESLint just assumes that the whole state is used becaused it passed as a prop is enough. Assuming that none of the fields is used later is wrong IMHO. |
You shouldn’t do it to satisfy eslint, you should do it because then someone reading this component doesn’t have to read a different component to figure out what your component’s API really is. Always explicitly destructure everything and re-provide it. |
@ljharb If by API you mean a set of component's props, then it can b easily described using prop-types. No need to pass each field separatelly just to show the API:
|
So you’re ok with duplicating the field name in both components’ propTypes, but not in the props themselves? |
In the container component the fields are state fields not props. So no duplicating in this case. |
What i mean then is, where you render Form, it should be explicit what props you’re passing without having to look at Form itself. |
i use antd config and react docs about context<ThemeContext.Provider value={this.state}> |
@Lliuxs never pass the state object directly. Explicitly destructure the properties you need out of the state, and pass a new object to the context provider. |
See related issue: jsx-eslint/eslint-plugin-react#1910 kiwicom-source-id: f619e017f84e955c11e356429abdd17a533e5bce
In the documentation's example constructor --> constructor(props) {
super(props);
this.toggleTheme = () => { // remove this and the lint error shows up
this.setState(state => ({
theme:
state.theme === themes.dark
? themes.light
: themes.dark,
}));
};
// State also contains the updater function so it will
// be passed down into the context provider
this.state = {
theme: themes.light,
toggleTheme: this.toggleTheme,
};
} They are essentially using the prop in the toggle function which removes this lint error. This is a bit confusing as when you are attempting to alter the docs example for your own and remove that toggle function the red squiggly reappears. I would love to see someone from the react team chime in here... Just wanted to add this, I will go back to my popcorn now and watch the comments... |
But you actually ain't using // eslint-disable-next-line react/no-unused-state
bar: 'baz', |
I'm getting a false positive:
[eslint] Unused state field: 'bar' (react/no-unused-state)
when using the following:
Is there a way to bypass this warning? (I'm using the latest version).
The following will bypass it:
<FooContext.Provider value={{ ...this.state }}>
However this will cause unintentional renders for all Context consumers
Thanks
The text was updated successfully, but these errors were encountered: