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

chore!: upgrade got to 11.8.5 #225

Merged
merged 5 commits into from Aug 10, 2022
Merged

Conversation

clavin
Copy link
Member

@clavin clavin commented Jun 29, 2022

Bumps the dependency on got to version 11.8.5, fixing a dependabot alert. got also now includes its own type definitions, so the dependency on @types/got could be removed as well.

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

I think this is a breaking change, as it raises the minimum Node.js version?

@malept
Copy link
Member

malept commented Jun 29, 2022

Also for some reason the tests haven't been triggered

@clavin
Copy link
Member Author

clavin commented Jun 29, 2022

Ah, you're correct. The requirement for got v11 is "node": ">=10.19.0", whereas this package is still "node": ">=8.6". Nice catch!

@clavin
Copy link
Member Author

clavin commented Jun 29, 2022

Added the change to the minimum supported node version requirement to this PR as well just to make sure it doesn't get lost, if we choose to merge this.

@malept
Copy link
Member

malept commented Jun 30, 2022

A couple of things:

  1. The PR title should reflect that this is a breaking change: chore!: upgrade got to 11.8.5
  2. I think you need to re-log-into CircleCI so that the tests will run. If I had to guess it's because of the multiple-orgs-that-require-SSO problem.

@clavin clavin changed the title chore: upgrade got to 11.8.5 chore!: upgrade got to 11.8.5 Jun 30, 2022
@clavin
Copy link
Member Author

clavin commented Jun 30, 2022

Looks like a dependency from this got version (compress-brotli) actually requires the min version to be >=12, even though got is itself only marked as >=10.19. Also have to bump the @types/node dependency here.

@clavin clavin requested a review from malept June 30, 2022 15:38
@vince-fugnitto
Copy link

Any update?

@malept
Copy link
Member

malept commented Jul 7, 2022

See #224 (comment), merging this change needs to be coordinated properly.

@BlackHole1
Copy link
Member

BlackHole1 commented Aug 9, 2022

Based on #226 (comment) discussions. I think we should just upgrade instead of waiting for got to release a new patch version.

cc: @malept

@malept
Copy link
Member

malept commented Aug 9, 2022

We're not waiting for got to release a new patch version.

@BlackHole1
Copy link
Member

What are the reasons that are stopping this PR from being merged now? I want to push for this PR to be merged.

The most important reason, as I understand it so far, is the impact of the minimum node version

@BlackHole1
Copy link
Member

BlackHole1 commented Aug 9, 2022

If this impact is so important to us that we cannot merge this PR in a short time, is there another way, for example, if we fork got

@erickzhao erickzhao requested a review from a team August 9, 2022 17:04
@malept malept merged commit a8bb6f1 into electron:main Aug 10, 2022
@electron-bot
Copy link
Contributor

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants