feat: Switch to new transports #4943

AbhiPrasad · 2022-04-14T16:48:22Z

This PR switches over from the old transports to the new ones. It deletes the functionality from the client of passing in transports explicitly and instead expects the transport to be passed in through options.

Instead of passing in an instance of a transport to the client, we pass in a constructor function. This allows the client to control exactly what options are passed into the transport.

this._transport = options.transport({ ...options.transportOptions, url });

Resolves https://getsentry.atlassian.net/browse/WEB-774

AbhiPrasad · 2022-04-19T02:47:17Z

packages/core/src/baseclient.ts

-      ...((this._transport as unknown as { _api: Partial<APIDetails> })._api || {}),
-      metadata: options._metadata || {},
-    };
+    this._transport = options.transport({ ...options.transportOptions, url: url || '' });


this is the key here, making sure the client still controls what is being passed in as options.

packages/browser/src/transports/new-fetch.ts

github-actions · 2022-04-19T16:09:49Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.86 KB (-1.04% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	63.09 KB (-2.26% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.54 KB (-1.29% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	56.81 KB (-1.98% 🔽)
@sentry/browser - Webpack (gzipped + minified)	21.77 KB (-6.2% 🔽)
@sentry/browser - Webpack (minified)	74.12 KB (-10.11% 🔽)
@sentry/react - Webpack (gzipped + minified)	21.81 KB (-6.21% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified)	46.21 KB (-3.72% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.67 KB (-1.2% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.09 KB (-1.31% 🔽)

lforst

Looking good. Some small things. We should definitely fix the tests before merging. Let me know if I can help there!

packages/core/src/transports/base.ts

packages/core/src/baseclient.ts

packages/node/src/transports/new.ts

lforst · 2022-04-20T15:42:02Z

packages/node/src/transports/setup.ts

-    ...options.transportOptions,
-    ...(options.httpProxy && { httpProxy: options.httpProxy }),
-    ...(options.httpsProxy && { httpsProxy: options.httpsProxy }),
-    ...(options.caCerts && { caCerts: options.caCerts }),


I think that's a good idea. That's what your changes do anyways right?

lforst · 2022-04-20T15:55:59Z

packages/node-integration-tests/suites/public-api/addBreadcrumb/simple_breadcrumb/test.ts


-  assertSentryEvent(requestBody, {
+  assertSentryEvent(envelopes[1][2], {


This is super wonky. We might want to write utility functions in the future to avoid these index shenanigans. (not this PR tho)

yes we can improve this API a lot, I'll write up a improvement ticket.

packages/node-integration-tests/suites/sessions/crashed-session-aggregate/test.ts

packages/types/src/transport.ts

lforst · 2022-04-20T16:03:32Z

MIGRATION.md

- Inject transport into client instead of initializing it in the client in `setupTransport` (see
-  [#4921](https://github.com/getsentry/sentry-javascript/pull/4921/)). If you are creating your own `Client` or
-  calling `initAndBind`, you will have to supply your desired transport. Either provide a custom one or call
-  `setupBrowserTransport` or `setupNodeTransport` for default transports, depending on your requirements.
 - Remove support for Opera browser pre v15


I think this PR might be a good opportuinity to add the transport changes to the migration docs.

Yeah agree, I'll add those.

Decided that I'll do this in a follow up PR!

Letting through requests with query params

lforst · 2022-04-21T17:00:05Z

Tests are finally passing! 🍾

@AbhiPrasad it would be great if you could check if the changes I made actually make sense.

Edit: Actually I'm not sure if ember is flaky again or if it has failing tests for real.

AbhiPrasad · 2022-04-21T18:26:28Z

Ember tests were for real! Fixed them - hopefully this should be it 🤞

Update: It wasn't :(

Lms24

Overall, this looks really good to me, nice work @AbhiPrasad and @lforst! All my questions/remarks are more or less asked out of curiosity which is why I think this is ready to go regardless of the answers.

Admittedly, I only skimmed over the tests but they also seemed fine to me. I do have a few questions related to tests:

It seems like we deleted the integration tests for the new transports (browser and node). Is there a reason for this?
Also, we deleted the transport setup unit tests (the ones that test correct setup of fetch and xhr, http/s transports). I mean those weren't the most critical tests for sure but wouldn't it still be good to test this behaviour? Or did I miss something here (i.e. were they moved to a different file)?

Nice bundle size reduction btw! 🚀

packages/core/src/baseclient.ts

lforst · 2022-04-22T12:21:00Z

@Lms24

It seems like we deleted the integration tests for the new transports (browser and node). Is there a reason for this?

I think this is due to the fact that we now don't have "new" transports anymore and the integration tests for the old transports now test the new ones. Keeping these tests would mean we would test stuff twice, which is unnecessary.

Also, we deleted the transport setup unit tests (the ones that test correct setup of fetch and xhr, http/s transports). I mean those weren't the most critical tests for sure but wouldn't it still be good to test this behaviour? Or did I miss something here (i.e. were they moved to a different file)?

We entirely removed the setup functions and their tests. This is what we discussed in #4921 (comment). I think these setup functions are redundant if we don't have to deal with the old transports and so would be their tests.

AbhiPrasad · 2022-04-25T03:07:54Z

Thanks for answering the questions ❤️ @lforst

Merging this in to unblock the rest of our work!

This PR switches over from the old transports to the new ones. It deletes the functionality from the client of passing in transports explicitly and instead expects the transport to be passed in through options. Instead of passing in an instance of a transport to the client, we pass in a constructor function. This allows the client to control exactly what options are passed into the transport. ```ts this._transport = options.transport({ ...options.transportOptions, url }); ```

AbhiPrasad mentioned this pull request Apr 18, 2022

feat(core): Introduce seperate client options #4927

Merged

AbhiPrasad force-pushed the v7-abhi-use-new-transport branch from 687cb99 to f282c6c Compare April 19, 2022 02:41

AbhiPrasad changed the base branch from 7.x to v7-options-from-client April 19, 2022 02:42

AbhiPrasad commented Apr 19, 2022

View reviewed changes

AbhiPrasad force-pushed the v7-abhi-use-new-transport branch from 3dc2618 to 5d528c3 Compare April 19, 2022 03:08

AbhiPrasad commented Apr 19, 2022

View reviewed changes

packages/browser/src/transports/new-fetch.ts Outdated Show resolved Hide resolved

AbhiPrasad force-pushed the v7-abhi-use-new-transport branch 3 times, most recently from 7e52ad5 to 870f57a Compare April 19, 2022 15:54

Base automatically changed from v7-options-from-client to 7.x April 19, 2022 16:46

AbhiPrasad and others added 19 commits April 19, 2022 13:35

feat(core): Introduce seperate client options

5745c33

introduce diff option types

fd4dcd8

introduce diff browser types:

d2e8764

node client options

f1701b4

switch browser

b77cd21

don't do anything with the dsn

7735f79

dsn type fix

523bdf7

remove default core options

5a64ba8

add comments

cd01213

cast option types

0d00e20

explicit types

b3cfe41

fix tests

8cb4fac

fix transport types

1463745

round 1 of fixing tests

4e1c76c

tmp fix core sdk.test.ts

71fbb8e

Add tests for integration setup in node init

35a8000

add tests for integration setup in browser init

a089559

Fix some eslint errors in node tests

b2bd018

Extract getDefaultNodeClientOptions into helper module

4fd61b6

lforst reviewed Apr 20, 2022

View reviewed changes

AbhiPrasad and others added 13 commits April 20, 2022 13:23

fix node release health tests

2d299d4

work toward fixing nextjs tests

7a86050

fix node integration tests

3d1a6a4

fix tracing tests

174362f

PR review

8f0bdbf

comment out test

fec138c

tracing fix

6d9eb92

Fix nextjs integration tests nocks

73d6c82

Letting through requests with query params

Fix copy paste error

c12a077

Fix next integration tests

626f261

Fix nextjs unit tests

29a2c7a

Fix linter errors

2f3105a

Fix browser integration tests by implementing timout function

d895160

AbhiPrasad added 2 commits April 21, 2022 13:38

Merge branch '7.x' into v7-abhi-use-new-transport

e2b1864

fix ember tests

f301815

only add 1 event processor

ac1db21

Lms24 approved these changes Apr 22, 2022

View reviewed changes

packages/core/src/baseclient.ts Show resolved Hide resolved

AbhiPrasad merged commit 6267b54 into 7.x Apr 25, 2022

AbhiPrasad deleted the v7-abhi-use-new-transport branch April 25, 2022 03:08

AbhiPrasad mentioned this pull request Apr 25, 2022

feat: Delete old transports #4967

Merged

AbhiPrasad added this to the 7.0.0 milestone Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Switch to new transports #4943

feat: Switch to new transports #4943

AbhiPrasad commented Apr 14, 2022 •

edited

Loading

AbhiPrasad Apr 19, 2022

github-actions bot commented Apr 19, 2022 •

edited

Loading

lforst left a comment

lforst Apr 20, 2022

lforst Apr 20, 2022

AbhiPrasad Apr 20, 2022

lforst Apr 20, 2022

AbhiPrasad Apr 20, 2022

AbhiPrasad Apr 21, 2022

lforst commented Apr 21, 2022 •

edited

Loading

AbhiPrasad commented Apr 21, 2022 •

edited

Loading

Lms24 left a comment •

edited

Loading

lforst commented Apr 22, 2022

AbhiPrasad commented Apr 25, 2022


		assertSentryEvent(requestBody, {
		assertSentryEvent(envelopes[1][2], {

feat: Switch to new transports #4943

feat: Switch to new transports #4943

Conversation

AbhiPrasad commented Apr 14, 2022 • edited Loading

AbhiPrasad Apr 19, 2022

Choose a reason for hiding this comment

github-actions bot commented Apr 19, 2022 • edited Loading

size-limit report 📦

lforst left a comment

Choose a reason for hiding this comment

lforst Apr 20, 2022

Choose a reason for hiding this comment

lforst Apr 20, 2022

Choose a reason for hiding this comment

AbhiPrasad Apr 20, 2022

Choose a reason for hiding this comment

lforst Apr 20, 2022

Choose a reason for hiding this comment

AbhiPrasad Apr 20, 2022

Choose a reason for hiding this comment

AbhiPrasad Apr 21, 2022

Choose a reason for hiding this comment

lforst commented Apr 21, 2022 • edited Loading

AbhiPrasad commented Apr 21, 2022 • edited Loading

Lms24 left a comment • edited Loading

Choose a reason for hiding this comment

lforst commented Apr 22, 2022

AbhiPrasad commented Apr 25, 2022

AbhiPrasad commented Apr 14, 2022 •

edited

Loading

github-actions bot commented Apr 19, 2022 •

edited

Loading

lforst commented Apr 21, 2022 •

edited

Loading

AbhiPrasad commented Apr 21, 2022 •

edited

Loading

Lms24 left a comment •

edited

Loading