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

Record fetch requests in breadcrumb #744

Merged
merged 1 commit into from
Oct 26, 2016

Conversation

romainneutron
Copy link
Contributor

Hello,

this is an integration of capturing fetch requests the same way XMLHttpRequests are captured for breadcrumb.

Tests are passing on my browser using node_modules/.bin/grunt run:test on http://localhost:8000/test/integration/
However, running node_modules/.bin/grunt test fails locally with

Running "mocha:integration" (mocha) task
Testing: test/integration/index.html

․․․․․․․․․․․․․․․․XMLHttpRequest cannot load https://example.com/api/1/store/?sentry_key=public. Origin file:// is not allowed by Access-Control-Allow-Origin.
․․․․․․․․․․․

  26 passing (5s)
  1 failing

  1) integration breadcrumbs should record a fetch request:
     timeout of 2000ms exceeded

I don't know why. If anyone has an idea...

@benvinegar
Copy link
Contributor

Sweet! I'll probably merge this as-is, but follow-up and change the config options so that xhr becomes http and includes both fetch and XMLHttpRequest.

@benvinegar
Copy link
Contributor

I don't know why. If anyone has an idea...

The tests are run in Phantom (1?), which probably doesn't have fetch. It likely errors (method does not exist) and never completes, hence the timeout.

@romainneutron romainneutron force-pushed the add-fetch-to-breadcrumb branch from 501b808 to db16c0c Compare October 19, 2016 11:01
@romainneutron
Copy link
Contributor Author

@benvinegar I just updated the PR and fixed the tests. I added the fetch polyfill to the test iframe but forgot the es6-promise polyfill to get it work correctly :/ So tests are now passing

About renaming fetch+xhr to http setting, are you sure this is a good idea? Sounds like a BC break for users. Anyway, if you're sure, I can make the change in this PR. Let me know

@benvinegar
Copy link
Contributor

About renaming fetch+xhr to http setting, are you sure this is a good idea? Sounds like a BC break for users.

We would make it backwards compatible.

For now, let's just keep the option "xhr" ... and have it encompass both XMLHttpRequest and fetch. That way, if someone has explicitly disabled the "xhr" option, they won't magically have "fetch" requests appear in their breadcrumbs when they upgrade to the next version.

@romainneutron romainneutron force-pushed the add-fetch-to-breadcrumb branch from db16c0c to 5dadbcf Compare October 20, 2016 11:57
@romainneutron
Copy link
Contributor Author

PR updated, should be alright now :)

@romainneutron
Copy link
Contributor Author

Anything left here?

Copy link
Contributor

@benvinegar benvinegar left a comment

Choose a reason for hiding this comment

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

Looks great, merging.

@benvinegar benvinegar merged commit 3671931 into getsentry:master Oct 26, 2016
@romainneutron romainneutron deleted the add-fetch-to-breadcrumb branch October 30, 2016 08:49
denstepa pushed a commit to denstepa/raven-js that referenced this pull request Jan 5, 2017
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.

2 participants