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

Add to support new eslint suggestions API #814

Merged
merged 2 commits into from Feb 17, 2020

Conversation

@ota-meshi
Copy link
Contributor

ota-meshi commented Nov 27, 2019

This PR makes the suggestions API added in ESLint v6.7 available via the quick fix.


close #806

@msftclas

This comment has been minimized.

Copy link

msftclas commented Nov 27, 2019

CLA assistant check
All CLA requirements met.

@dbaeumer dbaeumer added this to the 2.1.0 milestone Dec 2, 2019
@wdoug

This comment has been minimized.

Copy link

wdoug commented Dec 11, 2019

@dbaeumer, I see that this was assigned to the 2.1.0 milestone. I was just curious when this is expected to get released and if there is anything that could be done to help get it released soon?

@ota-meshi ota-meshi force-pushed the ota-meshi:supports_suggestions branch from d91abcd to 3e105b0 Jan 12, 2020
@wdoug

This comment has been minimized.

Copy link

wdoug commented Jan 19, 2020

@dbaeumer is there any way to get this pull request merged and released soon? Among other benefits to getting this, not having this functionality is currently blocking changes to the eslint-plugin-react-hooks (pull request) that would stop react-hooks/exhaustive-deps dangerously updating code (changing runtime behavior) when eslint's autofix is run. In turn, this means that that rule currently can't safely be used in projects that have violations of that rule.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Jan 20, 2020

@wdoug thanks for checking back. The goal for January is to stabilize the new 2.0.x version. I will start working on 2.1.x in Feburary.

@gaearon

This comment has been minimized.

Copy link

gaearon commented Feb 10, 2020

Hi @dbaeumer, is there anything on this PR that we or the community could help with? Thank you!

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Feb 17, 2020

I looked at the PR and it looks good, but I do have a couple of questions:

interface ESLintSuggestionResult {
	desc?: string;
        messageId?: string;
	fix: ESLintAutoFixEdit;
}

The PR however has no support for messageId and doesn't handle the case where desc is undefined. I have to say that I am not sure if this is a typing mistake in the ESLint API since I don't see any way how a client could resolve a messageId escpecially in the case where it uses placeholders (https://eslint.org/docs/developer-guide/working-with-rules#placeholders-in-suggestion-messages). Do you have any insight on this?

@ota-meshi

This comment has been minimized.

Copy link
Contributor Author

ota-meshi commented Feb 17, 2020

Hi @dbaeumer.

As an example, I tested on the no-useless-escape report.

/* eslint no-useless-escape: error */
'hol\a';

image

no-useless-escape seemed to build desc using messageId.

https://github.com/eslint/eslint/blob/master/lib/rules/no-useless-escape.js#L126
https://github.com/eslint/eslint/blob/master/lib/linter/report-translator.js#L204

I don't think desc can be undefined as far as I can see.

https://github.com/eslint/eslint/blob/master/lib/linter/report-translator.js#L266

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Feb 17, 2020

Hi @ota-meshi

thanks for the quick answer.

Looking at https://github.com/eslint/eslint/blob/master/lib/linter/report-translator.js#L266 I am not convinced that desc can't be undefined. If a meesageId is set it only checks that there is no desc set, but it doesn't convert the messageId into a desc.

I still think that a messageId will not leak to the caller since it will not be able to resolve it. So may be the easiest to prevent exception is to drop all suggestions that have no description (in the case they would leak).

Looking at the screen shot I would also suggest to remote the Fix this .... from suggestions because it results in very wide quick fix menus. We could add the rule id to the end of the desc.

@ota-meshi

This comment has been minimized.

Copy link
Contributor Author

ota-meshi commented Feb 17, 2020

I think that messageId is converted to desc in the following code.

https://github.com/eslint/eslint/blob/master/lib/linter/report-translator.js#L204

(I'm sorry if you don't communicate well because I don't understand English well.)

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Feb 17, 2020

Seems reasonable.

@ota-meshi

This comment has been minimized.

Copy link
Contributor Author

ota-meshi commented Feb 17, 2020

I changed the message displayed in the suggestions.

image

@dbaeumer dbaeumer merged commit 4749215 into microsoft:master Feb 17, 2020
2 checks passed
2 checks passed
Microsoft.vscode-eslint Build #20200217.1 succeeded
Details
license/cla All CLA requirements met.
Details
@ota-meshi ota-meshi deleted the ota-meshi:supports_suggestions branch Feb 17, 2020
@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Feb 17, 2020

I published a pre-release here. Please let me know who it works. Then I plan an official release end of the week.

@gaearon

This comment has been minimized.

Copy link

gaearon commented Feb 17, 2020

Thank you both for your work on this! It works great in my testing.

@wdoug

This comment has been minimized.

Copy link

wdoug commented Feb 17, 2020

FYI, for anyone that wants to test this before the official release, you can download the .vsix file from the release linked above. Then in the top-right of the VSCode extensions panel if you click the three dots ellipsis icon (...) you can select Install from VSIX... and install the extension from that .vsix file.

@shrugs

This comment has been minimized.

Copy link

shrugs commented Mar 2, 2020

for searchers that stumble onto this PR but can't find the current suggested workaround to get the autofix back: use eslint-plugin-react-hooks@2.3.0

facebook/react#17385 (comment)

@gaearon

This comment has been minimized.

Copy link

gaearon commented Mar 2, 2020

@shrugs The correct fix is to upgrade the VS Code plugin (2.1.0+) and ESLint (6.7.0+), not downgrade the ESLint plugin.

@shrugs

This comment has been minimized.

Copy link

shrugs commented Mar 2, 2020

@gaearon is there an option in the latest vscode-eslint or eslint itself to auto-apply the suggestions?

i wasn't able to find one, and my vscode is only suggesting fixes (in the lightbulb & cmd-. pane)

here are the versions i'm running:

$ yarn list | grep eslint
...
├─ eslint-plugin-react-hooks@2.5.0
├─ eslint@6.8.0
...
$ code --list-extensions --show-versions | grep vscode-eslint
dbaeumer.vscode-eslint@2.1.1
@bradzacher

This comment has been minimized.

Copy link

bradzacher commented Mar 2, 2020

There is not an option.
Suggestions are intentionally designed such that they cannot, and should not be automatically applied.

This is because:

  • a suggested fix may introduce potentially broken code, or change functionality of code
  • there may be multiple suggested fixes for a given eslint report (i.e. you can fix it this way, or this way).

The only way to apply a suggestion fix is manually via IDE tooling.

If you would like to see the react eslint plugin add an option to support fixers, please consider moving the discussion into an issue in that repo.

@gaearon

This comment has been minimized.

Copy link

gaearon commented Mar 2, 2020

This is intentional. We removed the fixer because the result has observably different behavior and can even lead to errors and infinite loops.

@shrugs

This comment has been minimized.

Copy link

shrugs commented Mar 2, 2020

i get that—entirely reasonable, especially re:eslint not wanting to materially change code execution.

for context, i find the auto-fix nature of the hooks dependencies core to my workflow and have only needed to opt-out with an eslint-ignore once or twice in my experience. probably 98% of the hooks code i write wants a fully-specified dependency array.

i imagine many other devs have a similar flow where they can assume the dependencies are correctly tracked by eslint with format-on-save, except in special cases (which, imo, is also a good code-smell for being able to structure the code better or more hooks-esq). the famous example of this rule being 'wrong' is useEffect(() => {}, []) for on-mount behavior, but are there other frequent examples? are there newer best practices around hooks dependencies where the previous behavior of implicitly including all of the references is no longer the vast majority of cases?

in any case, this is clearly the right change for the eslint ecosystem, but in terms of developer experience, imo we've lost something

@bradzacher which project would be best to create an issue on? i'm not very familiar with the structure of these projects

@bradzacher

This comment has been minimized.

Copy link

bradzacher commented Mar 3, 2020

If you'd like to talk to the relevant team about adding an option to convert the suggestion to a fix, the repo attached to the eslint plugin:
https://www.npmjs.com/package/eslint-plugin-react-hooks

If you'd like to talk to the relevant team about adding cli args to eslint to auto-fix suggestions, then the repo attached to eslint:
https://www.npmjs.com/package/eslint

@LeopoldLerch

This comment has been minimized.

Copy link

LeopoldLerch commented Mar 3, 2020

My opinion is, that it works absolutely right now. A dev should not have to opt-out of a process, to not break his code. He will either forget it or do it AFTER he had a bug. Also code-smells should be pointed to, not autofixed, after all, it is just a code-smell.

A developer should not trust a tool, to make a correct workflow for him. After all, that tool might change and create bugs in already existing code, that were not there before, as for example eslint is sometimes executed against the whole code, and rules for eslint can change. So you might have a bug in code, you did not touch for weeks.

As developer i always had to save, recheck the dependencies and verify again. It feels better now, i can ignore or apply the suggestions, but I can sure that the code does as i have written it. Just like suggestions for async/await for promises. I can do it that way, or leave the promises as they are.

@gaearon

This comment has been minimized.

Copy link

gaearon commented Mar 3, 2020

@shrug I think you may be missing my point. I absolutely agree you shouldn’t suppress the rule. The rule is usually right. However I do think that “autofilling” should be triggered by an actual action (like a keyboard shortcut). Which I believe is provided with Suggestions.

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

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.