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.

Sign up for GitHub

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

Enable react-hooks/exhaustive-deps eslint rules #24914

Merged
merged 5 commits into from
Feb 9, 2023
Merged

Enable react-hooks/exhaustive-deps eslint rules #24914

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5e2e459
Upgrade eslint and eslint-plugin-react-hooks and enable exhaustive-deps
kevin940726 Aug 29, 2020
1badefd
Add CHANGELOG entry
kevin940726 Sep 1, 2020
6a8447e
Update changelog with more detail
getdave Feb 9, 2023
fa90107
Alphabetise hooks
getdave Feb 9, 2023
8358467
Remove accidental prettier dep change introduced via rebase
getdave Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/eslint-plugin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,10 @@
- The bundled `eslint-plugin-react` dependency has been updated from requiring `^7.20.0` to requiring `^7.22.0` ([#27965](https://github.com/WordPress/gutenberg/pull/27965)).
- The bundled `eslint-plugin-react-hooks` dependency has been updated from requiring `^4.0.4` to requiring `^4.2.0` ([#27965](https://github.com/WordPress/gutenberg/pull/27965)).

### New Features
Copy link
Member

@gziolo gziolo Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changelog entry is misplaced. It should be moved to ## Unreleased section.

We will have to rethink the way changelogs are maintained, as it's too easy to run accidentaly into this type of situation when merging the latest changes from trunk.

Copy link
Contributor

@ciampo ciampo Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will have to rethink the way changelogs are maintained,

100%. I was thinking if we could semi-automate the changelog by asking the PR author to simply state:

  • which category this falls under (bug fix, enhancements, breaking change...)
  • message

The rest can be performed by the automated action at merge time, including picking the right CHANGELOG file and adding the message under the right version (or unreleased) ?

Copy link
Member

@gziolo gziolo Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at Jetpack monorepo the other day, and they use an interesting system.

They keep a changelog folder for every package:

Screenshot 2023-02-13 at 09 33 51

Every time a changelog entry is warranted (I don't know the rules), they create a new file:

Screenshot 2023-02-13 at 09 33 40

Those files are read and deleted at publishing time, which completely removes the risk of merge conflicts of any type.

Example PR from Jetpack with changelog entries included: Automattic/jetpack#28710.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting!

It could be worth chatting to whoever implemented / maintains that system to understand pros and cons, and if it could work well for our use case. For example, I see that they have a CI action that replies to the PR with a checklist, suggesting that a CHANGELOG entry is a mandatory requirement for all PRs?


- Enable `react-hooks/exhaustive-deps` rules in the react config.

## 7.4.0 (2020-12-17)

### New Feature
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/configs/react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,6 @@ module.exports = {
'react/prop-types': 'off',
'react/react-in-jsx-scope': 'off',
'react-hooks/rules-of-hooks': 'error',
'react-hooks/exhaustive-deps': 'warn',
getdave marked this conversation as resolved.
Show resolved Hide resolved
},
};
1 change: 1 addition & 0 deletions packages/eslint-plugin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"eslint-plugin-react": "^7.27.0",
"eslint-plugin-react-hooks": "^4.3.0",
"globals": "^13.12.0",
"prettier": "npm:wp-prettier@2.2.1-beta-1",
getdave marked this conversation as resolved.
Show resolved Hide resolved
"requireindex": "^1.2.0"
},
"peerDependencies": {
Expand Down