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

deps: ignore dependabot for peer dependencies #904

Merged
merged 1 commit into from
Oct 14, 2020

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Oct 14, 2020

Description

  • several of the dependencies in the tree are only there because they
    are peerDeps, and not because we directly depend on them

    • they should only be upgraded in tandem with the dep(s) they are
      peers of and pretty much never in isolation
  • Greenkeeper made several PRs for peers which previously caused an
    erroneous/buggy update of @types/jest to 25 many months before Jest
    was upgraded to 25

    • and honestly those Greenkeeper PRs for peers had confused me a
      good deal too

Tags

Let's close some of the confusing Greenkeeper peer update PRs now that this has been configured:

Review Notes

I straight copy+pasted the eslint-config-react-app peerDeps from its package.json, so they're alphabetized the same way and shouldn't have any spelling mistakes.

- several of the dependencies in the tree are only there because they
  are peerDeps, and not because we directly depend on them
  - they should only be upgraded in tandem with the dep(s) they are
    peers of and pretty much never in isolation

- Greenkeeper made several PRs for peers which previously caused an
  erroneous/buggy update of `@types/jest` to 25 many months before Jest
  was upgraded to 25
  - and honestly those Greenkeeper PRs for peers had confused me a
    good deal too
@vercel

This comment has been minimized.

@agilgur5 agilgur5 added scope: dependencies Pull requests that update a dependency file scope: internal Changes only affect the internal API labels Oct 14, 2020
Copy link
Collaborator Author

@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.

peerDeps all look to line up to me, but I think tslib should probably be included in here too because of #719 (comment).

But somewhat problematically, it doesn't seem to have a peerDep on typescript, meaning one wouldn't get a peerDep mismatch error if an incorrect version were installed. And that means it could be easy to forget to check/upgrade tslib in tandem with typescript... Hmmm, need to think about that.

This might be unintentional, so should check upstream in tslib and perhaps file a PR.

@agilgur5
Copy link
Collaborator Author

Seems like microsoft/tslib#22 exists that goes into this. microsoft/tslib#22 (comment) seemed against using peerDeps but without an example that was difficult to understand how the use-case actually plays out (does it mean in the dependency tree or a single package?)

@agilgur5
Copy link
Collaborator Author

Since older versions of tslib seem to be forward-compatible, it seems like the decision to upgrade tslib would be independent of TS upgrade as a result, so might not make sense to ignore it then.

Added a comment in the thread upstream, but think this is good to go as is due to above. Can always add more to the list as well.

@agilgur5 agilgur5 merged commit 569c3ed into jaredpalmer:master Oct 14, 2020
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Oct 14, 2020

Cleaned up ~74 Greenkeeper branches associated with @typescript-eslint/eslint-plugin, @typescript-eslint/parser, eslint-plugin-flowtype, eslint-plugin-import, eslint-plugin-react, eslint-plugin-react-hooks, @types/jest, and jest-watch-typeahead.

Think nearly all the rest can be cleaned up now too (~44 left). I think they're mostly minor version upgrades (hence no PR) or devDeps or both. Has taken a while to get here, have cleaned up 100s of Greenkeeper branches 😕

@agilgur5
Copy link
Collaborator Author

Deleted another ~12 Greenkeeper branches. They were mostly patch bumps of devDeps actually, a few minors and a few prod. Remaining ones seem to be all Prettier/eslint-config-prettier/eslint-plugin-prettier deps or template version bumps in the example dir

imgbot bot pushed a commit to mubaidr/tsdx that referenced this pull request Oct 31, 2020
- several of the dependencies in the tree are only there because they
  are peerDeps, and not because we directly depend on them
  - they should only be upgraded in tandem with the dep(s) they are
    peers of and pretty much never in isolation

- Greenkeeper made several PRs for peers which previously caused an
  erroneous/buggy update of `@types/jest` to 25 many months before Jest
  was upgraded to 25
  - and honestly those Greenkeeper PRs for peers had confused me a
    good deal too
paul-vd pushed a commit to EezyQuote/tsdx that referenced this pull request Dec 1, 2020
- several of the dependencies in the tree are only there because they
  are peerDeps, and not because we directly depend on them
  - they should only be upgraded in tandem with the dep(s) they are
    peers of and pretty much never in isolation

- Greenkeeper made several PRs for peers which previously caused an
  erroneous/buggy update of `@types/jest` to 25 many months before Jest
  was upgraded to 25
  - and honestly those Greenkeeper PRs for peers had confused me a
    good deal too
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 scope: internal Changes only affect the internal API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant