Request for Review: Adding node-fetch support to XRay
- Dominant language
- JavaScript
- Stars
- 280
- Forks
- 157
- PR merge metrics
- No merged PRs in 30d
Description
Hi, given that fetch is going to be in mainline NodeJS, not to mention it's easier to use than alternatives, I would like to see it supported in XRay. To that end, I was wondering if _somebody_ could take a look at my initial attempt and see if I'm on the right path. Most notably, I'm still a little fuzzy on what automatic versus manual mode entails. The code for my fetch capture routines in my forked repo is [here.](https://github.com/jasonterando/aws-xray-sdk-node/blob/fetch/packages/core/lib/patchers/fetch_p.js)
Some particulars:
1. The patch _should_ support both "built-in" Fetch in current NodeJS versions, as well as manually adding the node-fetch package.
2. Like fetch itself, the patched calls are set up for async/promises
3. I have not yet added Typescript defs (although will be easy to do)
4. Need to build unit testing around automatic/manual mode (once somebody confirms I'm taking an appropriate approach)
If somebody can either confirm it's looking okay, or give me guidance on what needs to be corrected, I can work on finishing the unit tests, TypeScript defs, etc. and submit a PR.
The full forked/branched repo is [here]
(https://github.com/jasonterando/aws-xray-sdk-node/tree/fetch)
Thanks in advance
Contributor guide
Research direction
Start with the proposed fetch capture routines in packages/core/lib/patchers/fetch_p.js from the linked fork, then review how automatic and manual modes are handled. Add support for built-in Fetch and manually installed node-fetch, cover both modes with unit tests, and add the missing TypeScript definitions before submitting a pull request.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, node.js
- Domain
- backend
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100