no-unused-variable

[tyv]

we don't have this rule yet. I vote to add it: as "no-unused-variable": true

[Zombiefruit]

I'd agree with it, if it wasn't for the pattern of removing certain props from being set on native elements. For example, with the image component, the props interface includes some custom props:

export interface ImageProps extends React.HTMLAttributes<HTMLImageElement> {
    ...
    defaultImage?: string;
    resizeMode?: 'fill' | 'cover' | 'contain';
   ...
}

and we don't want these props to be attributes on the rendered element. But a nice style for adding the props as attributes is: <img {...this.props} src={this.state.src} />.

In order to remove these props we don't want on the image element, we did:

const {
            // shouldn't be printed to DOM
            defaultImage,
            resizeMode,
            ...rest
        } = this.props;

and then rendering the image like <img {...rest} src={this.state.src} />.

Since the interface is gone at runtime, I don't see a cleaner way to do this. If you have a suggestion, I'm all ears.

[tyv]

@Zombiefruit maybe to use this https://github.com/tyv/pick-react-known-prop

[tyv]

@Zombiefruit also do you agree that it is not good pattern to destruct object with declaring variables only for reason to get rest of the object to spread it later?

[Zombiefruit]

Completely agree with you about it not being ideal. It's cool that you have a repo for this purpose already; perhaps we can combine that functionality into wix-react-tools since it's already iterating over the props?

[AviVahl]

Checked out pick-react-known-prop... There are several notes:

• Having to maintain a separate list of allowed HTML/SVG props is not ideal.
• No TypeScript typings for the package. These should probably use generics.
• Performance questions: validation of the input using lodash.isPlainObject (React always creates an object for you), regex testing for data- and aria- for each prop, etc.
• More tests... such as allowing data- and aria- for SVGs as well...

The above should all be discussed/addressed... and any solution should probably be exposed from wix-react-tools.

[tyv]

@AviVahl besides other valid inputs, lets use 'performance question' only when there is actually a question. Now it is not.

What do you suggest on the topic?

[tyv]

@Zombiefruit if you're agree, then don't use it. Use lodash.omit or something like that for each exact case. My opinion: it is way better.

[tobich]

I agree with @tyv that the "destructuring" pattern obscures its purpose and shouldn't be a reason for not having this rule.

[AviVahl]

I have no doubt it obscures the purpose, thus the comment was written... and yet, I feel like we keep assuming all these integrations (stylable-react-component, wix-react-tools, and the suggested lib) have no cost, while we should know better. All these repeated processing and re-processing of props have quite a run-time cost. And yes, it all happens during render, where we want to keep a low overhead.

Dismissing performance argument regarding a possible pattern, knowing what we know from previous endeavours, feels like ignoring our blind spot.

Anyhow, I agree the destructing pattern is not a blocker for this rule, even though I am personally not fond of it during development time.

[AviVahl]

btw, TypeScript also has this option built-in, and can be turned from tsconfig.json

[tyv]

btw, TypeScript also has this option built-in, and can be turned from tsconfig.json

I know, we had it before, but linting is 'checking while you write' I told about lib only as explanation of possibilities, you can use lodash omit or whatever

[alisey]

I am personally not fond of it during development time.

I feel the same way. I used to keep no-unused-variable enabled, but found it annoying and distracting during development. In the pre-commit hook I think it's fine.

but linting is 'checking while you write'

No, that isn't a conventional definition of linting.

[tyv]

i wasnt defining, i described how it is mostly used

[tyv]

I hear what you say, but since we don't yet have common tsconfig I found this argument 'very interesting'. thats better then nothing but not enough i think.

[alisey]

@tyv not sure if your comment was addressed to me, but can you elaborate on what you find very interesting, and what exactly is better than nothing?

[tyv]

@alisey we don't have common tsconfig, but we do have common lint rules, then if we want go further we have to deal with everyone on common tsconfig to handle unused variables or for leave it with tslint. That was part about 'better that nothing' I think it is easier to keep all lint rules in one place.

So interesting part is to try to make common tsconfig :) which may be hard.

[tyv]

In the pre-commit hook I think it's fine.

btw, you can disable auto-linting in your editor and also to set-up lint-staged package for library.

[AviVahl]

the only reason I mentioned TypeScript has it built-in, is because I already tried working with it. I wasn't suggesting it as an alternative. although, do note, it also gave live (red) feedback while I code (which is why I didn't like it). like you, I don't want any unused entities in my code, but that option was really annoying to work with. having it as default will most likely reduce my dev experience, so I vote no.

[tobich]

@AviVahl Do you think your reduced dev experience is related to the IDE "red line" (tsconfig) or by the rule in general? (tsconfig || ts-lint).

I have to say that unused variable is very often part of my code review comments; having it automated is very appealing to me. (Most important reason I voted +1)

That being said, I don't recollect having worked in this mode, so I can't say how it "feels" in practice. I am working in WebStorm and I see unused variables as grayed-out, so it draws attention enough to get rid of them, but doesn't interfere with the coding flow.

(Btw, if we implement it as part of the linting, we should generally encounter it only during final stages of feature development and PRs. As Bob Marley said: "You can run npm test sometimes, but you can't run npm test all the times.")

[amir-arad]

in Intellij / webstorm, you can auto-format an entire folder tree, including indentation and imports (combine and omit unused), so as long as the IDE's auto-formatting rules are precisely in sync with the linter, there's hardly any effort in doing it before pushing. I also agree on the improved readability of not allowing unused locals in the code.

I agree with @AviVahl on the reduced experience. working with noUnusedLocals and noUnusedParameters sometimes forces the developer to chase imports and declarations, after commenting out a single line for the sake of a small experiment.

So there's a big difference between PR-level linting and local on-the-fly linting

noUnusedParameters is a bit extra hard-core, too.

[tyv]

haha, nice points, never thought that. for me it is like red tests: you declare them and then make them green ;)

[amir-arad]

If we had a standard CI job that validates the lint rules as a separate process to the development cycle, and an IDE plugin that synchronizes the developer's IDE's auto-formatting rules with all the linting rules, I'd vote yes. I love the expected effect on git noise and readability.

[tyv]

@amir-arad do you use linting in IDE? I think we can set up lint-staged package for precommit hooks and then use more aggressive linting there

[AviVahl]

do you think your reduced dev experience is related to the IDE "red line" (tsconfig) or by the rule in general? (tsconfig || ts-lint).

@tobich when turned on from the tsconfig, I get the exact same behavior as I do from tslint (+vscode tslint extension). They are exposed as a red underline, just like any other error.

@tyv precommit hook would work. question is whether we want another config/setup just for this rule.

[tyv]

question is whether we want another config/setup just for this rule.

I don't :)

[amir-arad]

@tyv I try to avoid linting in general, I had no reason to install any lint plug-in in my IDE

[tyv]

good, then you will/may use CI/CLI for that.

[amir-arad]

=== bad experience, hence my downvote

[VKobeliatsky]

How's using CI is a bad experience? As for CLI, there are pre-commit hook solutions. We use linter to reassure consistent code style across the team. We should agree on the rules judging on how the code will look at the end. There are plenty of ways for the linter behavior to fit one's development experience.

[VKobeliatsky]

This was created 4 days ago. We are making decision on whether we add the rule on Monday. Speak now or be silent forever.

[AviVahl]

I wonder how many people who vote for this rule got to use it in the past...? My own opinion flipped only after I actually got to work with this turned on.

[idoros]

I voted yes, because no unused anything should be in the code, and I'm hoping it won't block build or anything like that during development.

[tyv]

I wonder how many people who vote for this rule got to use it in the past...?

I did. it is extremely helpful, especially when you refactor.

[idoros]

There are cases where you must have unused arguments because of function signature:

onDone((error, result) => { /* error is not used */
/* result is in use */
})

In typescript, when setting noUnusedParameters, arguments prefixed with _ are ignored.
I would like to add the same ignore pattern.

onDone((_error, result) => { /* error is not used, but ignored*/
/* result is in use */
})

[tyv]

we're checking, but eslint aloows to use any function parameter if it is before used.

[wtfil]

@Idobo this rule will not check function params, so feel free to use

onDone((error, result) => { /* error is not used */
/* result is in use */
})

[tyv]

@wtfil will it error when I don't use result either?

[wtfil]

@tyv no, it will not. I guess there is a separate rule for this

[VKobeliatsky]

The majority has voted for. PR merged.