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

Use compatible eslint-config-react-app and eslint-plugin-react-hooks. #890

Merged
merged 1 commit into from
Oct 5, 2020

Conversation

ludofischer
Copy link
Contributor

This solves the following warning on yarn install

[email protected]" has incorrect peer dependency "[email protected]".

@vercel

This comment has been minimized.

@ludofischer
Copy link
Contributor Author

I guess this is a breaking change for people who relied on the previous React hooks lint rules.

@agilgur5 agilgur5 added scope: dependencies Pull requests that update a dependency file version: minor Increment the minor version when merged labels Sep 28, 2020
@agilgur5
Copy link
Collaborator

Yeaaa, it's probably breaking, but I'm not really sure why eslint-config-react-app didn't release a breaking change for it, it seems to change the rules in patches and minors per the changelogs 😕 There's also no individual changelog for this package in its directory.

Per the milestone added here and my comments in #889 , I was planning to release all the ESLint and Prettier upgrades as one breaking change together.

Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

So the CI logs have a different warning now:

warning " > [email protected]" has incorrect peer dependency "[email protected] || 2.x".

🤔

But oddly enough they don't seem to show warnings for all the other peer deps that need upgrading if config-react-app is upgraded. Several more plugins would need to bumped per its current peerDeps

@agilgur5
Copy link
Collaborator

agilgur5 commented Sep 28, 2020

Ah, here's what the v5.2.1 peerDeps look like, which explains both the current warning and the lack of warnings. All the other changes (including addition of eslint-plugin-jest) seem to have been made after v5.2.1 and I guess not yet released?

EDIT: yes, they're only updated in the current pre-release line, @next

@ludofischer
Copy link
Contributor Author

In light of the discussion in #634 I wonder if it would not be better in the future to inline eslint rules into tsdx own config. That way it would not be forced to wrangle with dependency combinations which in this case are somewhat arbitrary (as package versions reflect rule changes more often than compatibility with eslint versions). Also you would have more control over which new warnings to introduce.

@agilgur5
Copy link
Collaborator

agilgur5 commented Oct 3, 2020

I'm not sure that would really solve anything and may make TSDX actually have to make more breaking changes, because most rule changes (at least those to errors) are breaking. I also personally don't want to have to wrangle with opinionated rules, the feedback therein, etc -- folks who make configs already deal with that and I'd prefer to use their work instead of adding more work for myself (which is already plenty). Better to make things more maintainable instead of less maintainable

package.json Outdated Show resolved Hide resolved
This solves the following warning on yarn install:

[email protected]" has incorrect peer dependency "[email protected]".
Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Minimally required change LGTM now. Double-checked and confirmed that there were no other breaking changes and basically no other changes in between 5.0.2 and 5.2.1

Thanks for the contribution!

@agilgur5 agilgur5 merged commit 57f7dcc into jaredpalmer:master Oct 5, 2020
@agilgur5
Copy link
Collaborator

agilgur5 commented Oct 5, 2020

@allcontributors please add @ludofischer for code

@allcontributors
Copy link
Contributor

@agilgur5

I've put up a pull request to add @ludofischer! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants