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

feat(node): Add new v7 http/s Transports #4781

Merged
merged 19 commits into from
Mar 29, 2022
Merged

Conversation

lforst
Copy link
Member

@lforst lforst commented Mar 25, 2022

This adds new http and https transports for using the new API discussed in #4660.

Fixes #2549 - we unify http and https in a single transport that differentiates based on the protocol that is used.

Ref: https://getsentry.atlassian.net/browse/WEB-728
Ref: https://getsentry.atlassian.net/browse/WEB-729

@lforst lforst changed the title feat(node): Add new v7 http/s Transports feat(node): Add new v7 node http/s Transports Mar 25, 2022
@lforst lforst changed the title feat(node): Add new v7 node http/s Transports feat(node): Add new v7 http/s Transports Mar 25, 2022
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

1st pass while I remember how node http API works lol

packages/node/src/transports/new.ts Show resolved Hide resolved
packages/node/src/transports/new.ts Outdated Show resolved Hide resolved
packages/node/src/transports/new.ts Outdated Show resolved Hide resolved
packages/node/src/transports/new.ts Outdated Show resolved Hide resolved
@lforst lforst marked this pull request as ready for review March 28, 2022 09:54
@github-actions
Copy link
Contributor

github-actions bot commented Mar 28, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.08 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 64.55 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.78 KB (-0.02% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 57.96 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 23.22 KB (0%)
@sentry/browser - Webpack (minified) 82.47 KB (0%)
@sentry/react - Webpack (gzipped + minified) 23.26 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.2 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.01 KB (-0.02% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.41 KB (-0.01% 🔽)

@lforst lforst marked this pull request as draft March 28, 2022 12:00
@lforst lforst marked this pull request as ready for review March 29, 2022 07:52
@lforst lforst requested a review from AbhiPrasad March 29, 2022 12:42
@@ -77,6 +77,11 @@ export class BrowserBackend extends BaseBackend<BrowserOptions> {
this._newTransport = makeNewFetchTransport({ requestOptions, url });
return new FetchTransport(transportOptions);
}

this._newTransport = makeNewXHRTransport({
url,
Copy link
Member

Choose a reason for hiding this comment

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

Think it’s a bad rebase, we’ll need to fix this!

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, should be resolved now

@lforst lforst force-pushed the lforst-node-transports branch 2 times, most recently from 8d17a98 to 22b0dba Compare March 29, 2022 12:56
@lforst lforst force-pushed the lforst-node-transports branch from 22b0dba to 53e0fc0 Compare March 29, 2022 13:03
@lforst lforst requested a review from AbhiPrasad March 29, 2022 13:05
packages/node/src/backend.ts Outdated Show resolved Hide resolved
packages/node/src/transports/new.ts Show resolved Hide resolved
packages/node/src/transports/new.ts Show resolved Hide resolved

const SERIALIZED_EVENT_ENVELOPE = serializeEnvelope(EVENT_ENVELOPE);

const unsafeHttpsModule: HTTPModule = {
Copy link
Member

Choose a reason for hiding this comment

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

why is this unsafe?

Copy link
Member Author

@lforst lforst Mar 29, 2022

Choose a reason for hiding this comment

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

It's unsafe because of the rejectUnauthorized: false option. CodeQL and Sonarlint are also complaining but since this is a test it should be fine.

@lforst lforst force-pushed the lforst-node-transports branch from 7803fbf to 339062c Compare March 29, 2022 16:16
@lforst lforst merged commit 58b3ddb into master Mar 29, 2022
@lforst lforst deleted the lforst-node-transports branch March 29, 2022 16:52
@AbhiPrasad AbhiPrasad added this to the Pre 7.0.0 Work milestone Apr 6, 2022
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.

@sentry/node transports
2 participants