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.

Sign up for GitHub

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

Jump to bottom

Node SDK - Vendor dependencies #9199

Closed
timfish opened this issue Oct 8, 2023 · 5 comments · Fixed by #10088
Closed

Node SDK - Vendor dependencies #9199

timfish opened this issue Oct 8, 2023 · 5 comments · Fixed by #10088
Labels
Package: node Type: Improvement

Comments

@timfish
Copy link
Collaborator

timfish commented Oct 8, 2023 •
edited

Problem Statement

@sentry/node has a few non-Sentry dependencies:

image

Solution Brainstorm

Vendor them!
@timfish timfish mentioned this issue Oct 8, 2023
Closed
@Lms24 Lms24 mentioned this issue Oct 10, 2023
Merged
@lforst
Copy link
Member

lforst commented Oct 16, 2023

This makes me think we can just remove tslib 🤔

@AbhiPrasad AbhiPrasad mentioned this issue Oct 18, 2023
Merged
@AbhiPrasad
Copy link
Member

I attempted to remove tslib in #5331, there was also #5753.

Let's give it a try again #9299

This was referenced Oct 18, 2023
Merged
Merged
@timfish
Copy link
Collaborator Author

timfish commented Dec 14, 2023

I looked at vendoring the last dependency http-proxy-agent but it's quite behind the latest version. It might be worth updating before vendoring but I wasn't sure if there is a reason it hasn't been updated?

@AbhiPrasad
Copy link
Member

I think we just forgot about updating it - let's update it first and then remove the dep.

@timfish
Copy link
Collaborator Author

timfish commented Dec 18, 2023

I've created a PR to vendor the latest version (v7) of https-proxy-agent but I think it may have changed supported Node versions by then. It has moved repository and the changelogs don't appear complete but it uses global URL and for await...of so I'm guessing it dropped support for Node < v10.

This would explain why it hasn't been updated in the Node SDK!

Probably best to merge this change with the next major rather than spend the time trying to vendor the older (non-typescript) version.

This was referenced Jan 5, 2024
Closed
Merged
AbhiPrasad added a commit that referenced this issue Jan 18, 2024
@timfish @AbhiPrasad
feat(node): Update and vendor https-proxy-agent (#10088)
acf58d3 
Closes #9199

This PR vendors the `https-proxy-agent` code and in the process updates
to v7.0.0.

`https-proxy-agent` is our last remaining cjs-only dependency so this is
required for #10046.

This removes the following dependencies:
- `https-proxy-agent@5.0.1`
- `agent-base@6.0.2`
- `debug@4.3.4`
- `ms@2.1.2`

The vendored code has been modified to use the Sentry logger rather than
`debug`.

Initially, rather than modify the vendored code substantially just to
pass our tight lint rules, I've disabled a few of the less important
lints that would make it particularly tricky to pull in upstream bug
fixes:
```ts
/* eslint-disable @typescript-eslint/explicit-member-accessibility */
/* eslint-disable @typescript-eslint/member-ordering */
/* eslint-disable jsdoc/require-jsdoc */
``` 

## Min supported Node version

`https-proxy-agent` has a `@types/node@14.18.45` dev dependency but
apart from adding an import for `URL`, I can't find anything that would
stop it working on older versions of node and there is nothing in the
changelogs that would suggest the min supported version has changed.

---------

Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Package: node Type: Improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(node): Update and vendor https-proxy-agent timfish/sentry-javascript
3 participants
@timfish @lforst @AbhiPrasad