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 node-fetch for fetchJSONFromURL #11

Merged
merged 3 commits into from
Feb 18, 2021

Conversation

dennisameling
Copy link
Contributor

As requested in git-for-windows/git#3040 (comment):

I have a feeling that this is hodgepodge code, and could be done much more elegantly

Actually, using node-fetch is a lot easier in this case. It makes mocking much easier as well. It's a very lightweight package that also adds convenience methods like res.json() to parse JSON responses.

@dscho
Copy link
Member

dscho commented Feb 18, 2021

Actually, using node-fetch is a lot easier in this case

Ironically, I did use it originally. But I was wary of adding yet more stuff that I do not really need.

Could you add an npm run build && npm run package commit, and compare the output size?

package.json Show resolved Hide resolved
src/downloader.ts Outdated Show resolved Hide resolved
src/downloader.ts Outdated Show resolved Hide resolved
src/downloader.ts Outdated Show resolved Hide resolved
Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

Very nice!

@dscho dscho merged commit f21a50c into git-for-windows:main Feb 18, 2021
@dscho
Copy link
Member

dscho commented Feb 18, 2021

I only reordered the commits so that dist/ is once again updated only in the end, in a dedicated commit.

@dscho
Copy link
Member

dscho commented Feb 18, 2021

Oh, completely forgot: thank you so much!

@dennisameling dennisameling deleted the node-fetch branch February 18, 2021 12:19
@dscho
Copy link
Member

dscho commented Feb 18, 2021

@dennisameling look at this beauty: https://github.com/marketplace/actions/setup-git-for-windows-sdk

@dennisameling
Copy link
Contributor Author

Love it!! 🚀 🎉

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.

2 participants