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(browser): Use proxy for XHR instrumentation #5843

Closed

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
Member

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
Member

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 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
Member

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants