Skip to content

feat(browser): Use proxy for XHR instrumentation#5843

Closed
Rockergmail wants to merge 1 commit intogetsentry:masterfrom
Rockergmail:feature/refactor-proxy-xhr
Closed

feat(browser): Use proxy for XHR instrumentation#5843
Rockergmail wants to merge 1 commit intogetsentry:masterfrom
Rockergmail:feature/refactor-proxy-xhr

Conversation

@Rockergmail
Copy link
Contributor

@Rockergmail Rockergmail commented Sep 28, 2022

the current implement of instrumentXHR can not cover:

  1. third party like axios, in lower version, onreadystatechange = function(){...} will cover the onreadystatechange function of sentry causing the problem of Bug: no xhr breadcrumbs for error in onreadystatechange #1333 because of prototype chain mechanism
  2. instrumentXHR as a proxy without cover users or third partys' implement, especially onreadystatechange function.

to solve these two problem, I commit this pr.

@Rockergmail Rockergmail changed the title feat: factor to proxy instrumentXHR support proxy Sep 28, 2022
@Rockergmail Rockergmail changed the title instrumentXHR support proxy chore: instrumentXHR support Proxy Sep 28, 2022
@Rockergmail
Copy link
Contributor Author

The solution of #2643 to fix #1333 is a good idea. But what if users have their own onreadystatechange and do not want to be covered ? I think the better solution is Proxy which can do really proxy things. By this way, we can keep onchangestatechange of users and third party like axios.

@Rockergmail
Copy link
Contributor Author

Rockergmail commented Sep 28, 2022

But I stuck in the testing. I am newbee about unit testing. I tried install dependancy both in the root dir and the package/browser. But after I ran yarn test in package/browser, it always output failed with :

yarn run v1.22.19
$ run-s test:unit
$ jest
 FAIL  test/unit/transports/xhr.test.ts
  ● Test suite failed to run

    test/unit/transports/xhr.test.ts:1:42 - error TS2307: Cannot find module '@sentry/types'.

    1 import { EventEnvelope, EventItem } from '@sentry/types';
                                               ~~~~~~~~~~~~~~~
    test/unit/transports/xhr.test.ts:2:51 - error TS2307: Cannot find module '@sentry/utils'.

    2 import { createEnvelope, serializeEnvelope } from '@sentry/utils';
                                                        ~~~~~~~~~~~~~~~
    test/unit/transports/xhr.test.ts:9:3 - error TS2322: Type '{ url: string; recordDroppedEvent: () => undefined; textEncoder: TextEncoder; }' is not assignable to type 'BrowserTransportOptions'.
      Object literal may only specify known properties, and 'url' does not exist in type 'BrowserTransportOptions'.

    9   url: 'https://sentry.io/api/42/store/?sentry_key=123&sentry_version=7',
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    test/unit/transports/xhr.test.ts:67:85 - error TS2339: Property 'url' does not exist on type 'BrowserTransportOptions'.

    67     expect(xhrMock.open).toHaveBeenCalledWith('POST', DEFAULT_XHR_TRANSPORT_OPTIONS.url);

I have no idea how to fix these and let the test go on. PLEASE HELP.

@lforst
Copy link
Contributor

lforst commented Sep 28, 2022

Hi, thanks for contributing! Can you summarize in the PR description what this PR is doing? Thank you!

@Rockergmail
Copy link
Contributor Author

@lforst updated.

@lforst
Copy link
Contributor

lforst commented Sep 28, 2022

Thank you! We will take a look and see if we can help with the tests!

@lforst lforst self-assigned this Sep 28, 2022
@lforst lforst added Package: browser Issues related to the Sentry Browser SDK Status: Backlog labels Sep 28, 2022
@lforst lforst self-requested a review October 4, 2022 08:36
@lforst lforst changed the title chore: instrumentXHR support Proxy feat(browser): Use proxy for XHR instrumentation Oct 27, 2022
@lforst
Copy link
Contributor

lforst commented Oct 27, 2022

Hi, I am deeply sorry for taking so long to get back to you.

I took some time to think about this PR - mainly about the bundle size impact vs. actual gain.

For now, I've decided that it's not worth the effort + bundle size impact to go through with this, especially since newer versions of axios already work. Some additional reasons supporting this decision:

  • Required maintenance
  • Work we would have to put into tests
  • We do monkey-patching like this in a lot of places and we haven't decided to do proxying there yet either.

Anyhow, I still want to thank you for your contribution. If you still want to see this go through, I recommend opening a feature request issue and if enough people want to see something similar we might reconsider.

@lforst lforst closed this Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Package: browser Issues related to the Sentry Browser SDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants