-
Notifications
You must be signed in to change notification settings - Fork 1
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: switch to cross-fetch from request #7
Conversation
@chohmann Not sure if you can review or someone else can, this would help resolve a lot of dependency pollution in apps like ours that don't use the |
@marbemac Can you review this or do you know who can? Thanks! |
@marbemac, @wmhilton, @daniel-white, @lottamus, @mmiask Can anybody please review/check this PR as the request package is deprecated for over 2 years now |
Should we make this use an isomorphic version to run in the browser? |
lib/cli/in.js
Outdated
fetch(url) | ||
.then(function (response) { | ||
if (response.status === 200) { | ||
callback(body); |
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.
where is body coming from now? we'll need to use the response.text()
or equivalent here
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.
Good call - I'll fix that up. I guess I'm too used to Typescript where I would have gotten a type error 😁
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.
Should be fixed now! I also spent a lot of time making sure the unit tests actually worked - they would pass previously but they weren't actually working due to the way stdout and stderr were being passed back in the test helper function. I added in a test express server that would allow us to verify that fetch is bringing back the correct data.
@daniel-white Thanks for reviewing! This is a CLI as far as I can tell, so I don't think it will ever run in a browser. That said, I don't mind using |
@strmer15 |
…eading the request body; use cross-fetch instead of node-fetch; fix the unit tests and add in a test server; remove old stubby config that wasn't used; fix reading from stdin
@daniel-white done! Let me know if you have any other concerns with this PR, thanks for reviewing! |
🎉 This PR is included in version 1.0.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Motivation and Context
This library currently uses the
request
library for its CLI usage, butrequest
has been deprecated for over 2 years, which means that all projects that depend on it also installrequest
, which includes all users of@stoplight/prism-cli
.cross-fetch
is an isomorphic implementation of thefetch
API that is not deprecated and provides very similar functionality.Description
I removed the
request
dependency, added in thecross-fetch
dependency, removedyarn.lock
and then ranyarn install
. I replaced therequire('request')
withrequire('node-fetch')
, switched therequest(...)
invocation tofetch(...).then(...).catch(...)
, and pulled out the common error handling code into a separate function. I uncommented all the CLI tests, fixed them, then ranyarn test
to verify they all passed.How Has This Been Tested?
Ran
yarn test
to verify everything passed locally, and uncommented the CLI tests to verify they worked.Types of changes
Checklist