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

fix(deps): add downstream peer dependencies #120

Closed
wants to merge 1 commit into from

Conversation

nicholaschiang
Copy link

Fixes #6 by adding downstream peer dependencies to this package (we add
the React specific dependencies as optional).

This also enables Yarn v2 support (which requires upstream packages to
include their dependency's peer dependencies).

Fixes iamturns#6 by adding downstream peer dependencies to this package (we add
the React specific dependencies as optional).

This also enables Yarn v2 support (which requires upstream packages to
include their dependency's peer dependencies).
Copy link
Owner

@iamturns iamturns left a comment

Choose a reason for hiding this comment

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

Great PR, thank you! Just one question about a version number. Cheers!

"@typescript-eslint/eslint-plugin": "^3.6.1"
"@typescript-eslint/eslint-plugin": "^3.6.1",
"eslint": "^5.16.0 || ^6.8.0 || ^7.2.0",
"eslint-plugin-import": "^2.21.2",
Copy link
Owner

Choose a reason for hiding this comment

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

Curious where this version came from?

In both eslint-config-airbnb and eslint-config-airbnb-base they reference ^2.22.0

Copy link
Author

Choose a reason for hiding this comment

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

I got that version from the warning logs when installing with Yarn V2 PNP.

Copy link
Owner

Choose a reason for hiding this comment

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

Looks like the peer dependency version was updated pretty recently in eslint-config-airbnb. Maybe the warning log you referenced was slightly outdated. Can you update this line ^2.22.0?

Copy link

Choose a reason for hiding this comment

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

This will be a bit hard to maintain. To mimic other libraries' peer dependencies will just go out of sync with time. I think it's cleaner to deal with this problem to decouple this library's dependencies from airbnb as much as possible and instead provide guidance on how to deal with these.
An alternative approach is proposed here #201

@stalniy
Copy link

stalniy commented Oct 21, 2020

this doesn't help, because this repo doesn't depend on :

    "eslint-plugin-jsx-a11y": "^6.3.0",
    "eslint-plugin-react": "^7.20.0",
    "eslint-plugin-react-hooks": "^4 || ^3 || ^2.3.0 || ^1.7.0"

But eslint-config-airbnb does, please read my comment

@iamturns
Copy link
Owner

I don't think this PR is relevant anymore, especially with the new dependencies introduced in #229

Thanks @stalniy

@iamturns iamturns closed this Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Got npm install Warnings When Use eslint-config-airbnb-base
4 participants