-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
ref(backend): Port functionality from Backend to Client #4911
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
For some reason, these two old browser integration tests keep failing: sentry-javascript/packages/browser/test/integration/suites/api.js Lines 23 to 62 in 4e722eb
Seems like |
62ec8da
to
99bde12
Compare
packages/core/src/baseclient.ts
Outdated
@@ -582,7 +654,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement | |||
this._updateSessionFromEvent(session, processedEvent); | |||
} | |||
|
|||
this._sendEvent(processedEvent); | |||
this.sendEvent(processedEvent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.sendEvent(processedEvent); | |
this._sendEvent(processedEvent); |
This should have stayed _sendEvent
, which then itself will call sendEvent
. By bypassing _sendEvent
, you're also bypassing the super._sendEvent
call, so event breadcrumbs are never getting attached. That's why your tests are failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Let's make sure we address this when we get to reviewing the client inheritance chain + looking at all the indirection in the SDK (part 6). We can maybe make the event processing chain smaller and easier to understand.
I made some tickets that went over this.
6fbf745
to
67563bb
Compare
port assertions checking TestBackend to inspect TestClient instead. Adjust spies to spy on ported functionality of TestBackend in TestClient
mark more removal TODOs
calling `sendEvent()` instead of `_sendEvent()` in `BaseClient` does not leads to the super classe's `_sendEvent()` not being called.
67563bb
to
3ce8093
Compare
port all the functionality from the Backend classes to the Client classes. This includes: * Backend (interface) * BaseBackend * BrowserBackend * NodeBackend * TestBackend Additionally, fix the unit and integration tests in Core to spy on TestClient instead of TestBackend.
port all the functionality from the Backend classes to the Client classes. This includes: * Backend (interface) * BaseBackend * BrowserBackend * NodeBackend * TestBackend Additionally, fix the unit and integration tests in Core to spy on TestClient instead of TestBackend.
port all the functionality from the Backend classes to the Client classes. This includes: * Backend (interface) * BaseBackend * BrowserBackend * NodeBackend * TestBackend Additionally, fix the unit and integration tests in Core to spy on TestClient instead of TestBackend.
port all the functionality from the Backend classes to the Client classes. This includes: * Backend (interface) * BaseBackend * BrowserBackend * NodeBackend * TestBackend Additionally, fix the unit and integration tests in Core to spy on TestClient instead of TestBackend.
port all the functionality from the Backend classes to the Client classes. This includes: * Backend (interface) * BaseBackend * BrowserBackend * NodeBackend * TestBackend Additionally, fix the unit and integration tests in Core to spy on TestClient instead of TestBackend.
This PR ports all the functionality from the
Backend
classes to theClient
classes. This includes:Backend
(interface)BaseBackend
BrowserBackend
NodeBackend
TestBackend
Additionally, the PR fixes the unit tests in Core to spy on
TestClient
instead ofTestBackend
. Furthermore, all places where the Backend classes should be removed are marked with aTODO(v7)
comment. The removal of the Backend classes will happen in a separate PR.Ref: