-
Notifications
You must be signed in to change notification settings - Fork 0
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
Move dependencies from peerDependencies to dependencies #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I don't think moving from peer to non-peer dependencies will cause problems.
package.json
Outdated
"peerDependencies": { | ||
"@typescript-eslint/eslint-plugin": "^1.11.0", | ||
"@typescript-eslint/parser": "^1.11.0", | ||
"peerDependencies": {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed.
'error', | ||
'ignorePackages', | ||
{ | ||
'js': 'never', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find it now but I remember reading the spec requires extensions for import. Any reason for disallowing extensions? What about ignoring the rule instead for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Keeping deps up to date is important and I appreciate you taking the time to do this 🙌
I tried this locally, and I had no issues with just upgrading the packages, but keeping them in peerDependencies 🤔 Maybe you encountered a different problem? Or is it because I don't have eslint
installed globally?
After seeing and agreeing with @kmkr's comment, I suggest that you split your commit into 2 (or three):
Upgrade packages
Move from deps to peerDeps
typescript: update import/extensions
, and then add a rationale for why this was done 👍
I guess you could do 1 and 2 in one go – you decide :)
7f8e30c
to
c522a9b
Compare
Upgraded all dependencies. Merging this now ✌️ Will publish to npm as soon as we're in master |
Thanks for dragging this across the finish line @rix1 ❤️ |
In Rainbow PR #21 I updated Rainbows dependencies. As part of that I upgraded ESLint from v5 to v6.
This change seemed to break the ESLint setup. The v6 migration guide seemed to indicate that plugins must be installed locally now. Thus, the dependencies was moved out of
peerDependencies
and intodependencies
. This fixed my problem.I'm not quite sure about the implications of this change, though. Feedback (and alternative solutions) are greatly appreciated. 🙂