search

moxystudio / eslint-config

MOXY eslint configuration to be used across several JavaScript projects
MIT License
13 stars 5 forks source link

Add eslint-plugin-react-hooks #70

Closed acostalima closed 5 years ago

satazor commented 5 years ago

The PR is good but I don’t think we need another addon for it, and simply enable it as part of the react addon. What do you think?

acostalima commented 5 years ago

The PR is good but I don’t think we need another addon for it, and simply enable it as part of the react addon. What do you think?

Yeah, I guess. The reason I didn't do that right now is because there may be some projects around that still do not make use of Hooks. In such cases, activating the plugin would be for nothing. The "downside" is that developers will have to explicitly declare the add-on in their project's config.

Should we still enable the plugin in the React add-on?

acostalima commented 5 years ago

The pipeline is breaking in Node v6 environment. The plugin only supports v7 onwards. Should we drop support for v6?

satazor commented 5 years ago

Enabling a new plugin even if it’s not doing nothing is not that bad, it will decrease performance just a little bit.

I think this all boils down to expectations: as a developer, I expect to have hooks linting by enabling the react addon, especially because hooks are part of the official react features now.

Can you change this PR to do that? It will actually be simpler.

satazor commented 5 years ago

Yep we can drop it, but add it as a breaking change.

satazor commented 5 years ago

Also, as part of a major bump, it would be nice to update eslint to v6, but I don’t know the amount of work it will give us.

acostalima commented 5 years ago

Enabling a new plugin even if it’s not doing nothing is not that bad, it will decrease performance just a little bit.

I think this all boils down to expectations: as a developer, I expect to have hooks linting by enabling the react addon, especially because hooks are part of the official react features now.

Can you change this PR to do that? It will actually be simpler.

I thought as much on the performance side of things, but I wasn't entirely sure. Anyway, I think you're right about developer's expectations.

I had something else in mind that I'd like to discuss. Since Hooks are really moving forward as part of React's ecosystem, should we consider enabling a rule in eslint-plugin-react to give a warning (or even an error) as an indication to favor function components instead of class components?

Yep we can drop it, but add it as a breaking change.

Will do.

Also, as part of a major bump, it would be nice to update eslint to v6, but I don’t know the amount of work it will give us.

That will cost us two breaking changes, but maybe we should address that in another pull request? 🤔

satazor commented 5 years ago

Yes lets address the eslint bump in another PR, but perhaps launch both commits in a single release.

In regards to the warning to prefer functional: we already had that enable a year ago or so, and we ended up disabling it for now. Perhaps this is something we can discuss as a team in an issue.

codecov[bot] commented 5 years ago

Codecov Report

Merging #70 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #70   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          21     23    +2     
  Lines          25     27    +2     
=====================================
+ Hits           25     27    +2
Impacted Files Coverage Δ
rules/react-hooks.js 100% <100%> (ø)
addons/react-hooks.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a4673e3...3cecd8f. Read the comment docs.

codecov[bot] commented 5 years ago

Codecov Report

Merging #70 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #70   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          21     22    +1     
  Lines          25     26    +1     
=====================================
+ Hits           25     26    +1
Impacted Files Coverage Δ
addons/react.js 100% <ø> (ø) :arrow_up:
rules/react-hooks.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a4673e3...6ace192. Read the comment docs.

acostalima commented 5 years ago

Yes lets address the eslint bump in another PR, but perhaps launch both commits in a single release.

In regards to the warning to prefer functional: we already had that enable a year ago or so, and we ended up disabling it for now. Perhaps this is something we can discuss as a team in an issue.

Alright! I don't think I have the needed permissions to change Travis' pipeline config. Can you do it?

acostalima commented 5 years ago

Nevermind my previous comment. I have just updated .travis.yml.