refactor(node): moving to native fetch
#1053
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erunion
added
enhancement
erunion
commented
Jul 28, 2024
| @@ -1,4 +1,4 @@ | |||
| import type { GroupingObject } from '../../src'; | |||
There was a problem hiding this comment.
GroupingObject doesn't exist in src/index.ts!
There was a problem hiding this comment.
went through and made a bunch of type fixes in #1056
erunion
commented
Jul 28, 2024
Comment on lines
-17
to
-19
| // We have to explicitly list these config files | ||
| // due to the dynamic require in certain envs | ||
| "src/config/*.json" |
There was a problem hiding this comment.
We haven't had any JSON files on src/config for a very long time.
kanadgupta
approved these changes
Jul 29, 2024
| @@ -1,4 +1,4 @@ | |||
| import type { GroupingObject } from '../../src'; | |||
There was a problem hiding this comment.
went through and made a bunch of type fixes in #1056
## 🧰 Changes Noticed a bunch of type errors when running `npx tsc -p test/` from the `packages/node` directory. Not sure if we should do this typechecking in CI? ## 🧬 QA & Testing Do you notice any errors when running `npx tsc -p test/` from the `packages/node` directory?
gratcliff
approved these changes
Jul 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🐳 Context
The amount of dependencies we use to make a
fetchcall to both the ReadMe and Metrics APIs is a bit off the rails right now:node-fetchfor the POST request to record a log.api@6which uses not onlynode-fetch, by way ofisomorphic-fetch, but it also usesoas,@readme/oas-to-har,fetch-har,json-schema-to-ts, but also our OpenAPI definition to make very simple GET requests to retrieve project and version data.api@7wouldn't be much of a change because that still uses everything listed her butnode-fetch. The complexity overhead for these two small API calls is a bit much.api@6also uses a very old version of ouroaslibrary and the work involved to upgrade that here, without just outright moving over toapi@7is non-trivial. It's easier to move off it entirely.1So I'm refactoring all of this away.
🧰 Changes
node-fetchfor nativefetch.fetchis readily available to us.apifor ReadMe API calls for nativefetch.json-schema-to-ts.mswto the latest release as the v2 release better supports nativefetch.form-dataas we have access a nativeFormDataAPI.2multertoo because in the tests it was used it wasn't actually being used.caseless.@types/har-formatto being a dev dependency as we're only using it for types and aren't exporting anything from it for external use.Footnotes
There's something to be said for dogfooding our own SDK software here but because we are unable to do so in any other SDK language, and
apiis unoptimized due to its design language requiring consulting an OpenAPI definition before every request IMHO it's not worth it for something as mission critical as our Metrics SDKs. ↩Not to mention one that actually follows the
FormDataspec. Don't get me started... ↩