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

Add fetchParameters to configure fetch #1177

Merged
merged 3 commits into from
Dec 20, 2017
Merged

Add fetchParameters to configure fetch #1177

merged 3 commits into from
Dec 20, 2017

Conversation

RuslanZavacky
Copy link
Contributor

@RuslanZavacky RuslanZavacky commented Dec 18, 2017

This PR adds the ability to configure fetch parameters.

Reference: https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch#Parameters

By default, we set keepalive: true and credentials: true. Keepalive should help to send request even on unload and credentials will pass cookies to the request. Passing cookies are essentials for self-hosted solutions if they are behind authorization that requires cookies (our use case).

Added referrerPolicy: 'origin' as well, as sentry requires referer header to be passed. Without this option it won't work, so making it default seems like a good idea.


  • If you've added code that should be tested, please add tests.
  • If you've modified the API (e.g. added a new config or public method), update the docs and TypeScript declaration file.
  • Ensure your code lints and the test suite passes (npm test).

@RuslanZavacky
Copy link
Contributor Author

This kinda covers #1172

@RuslanZavacky
Copy link
Contributor Author

Build failure here is a legit, as it relates to the #1149 merge.

And it seems its because of the chromium bug, that it does not return correct resourceType.

https://bugs.chromium.org/p/chromium/issues/detail?id=765501

@kamilogorek
Copy link
Contributor

I'm not very keen to add more and more config options. Especially that we already have transport attribute which can achieve all of this. In addition to that, it'd make fetch and XHR two completely separate entities, and they should always work in-line, as we default to XHR if no fetch is available.

Setting sane defaults is totally fine, but I'm not convinced for the rest. @MaxBittker @HazAT what do you think about it guys?

@RuslanZavacky
Copy link
Contributor Author

RuslanZavacky commented Dec 19, 2017

@kamilogorek headers & fetchParameters are different, as headers can be used for fetch and for XHR. transport is not really friendly, as you have to overwrite the whole logic, and to support that between updates is really hard. XHR already supported auth and passing cookies.

When fetch was added initially in another PR, passing parameters should've been implemented as fetch initially is too 'limited' to work out of the box. Which makes it more secure, but limited.

As a summary, in the current state, for more sophisticated setups, eg. hosted sentry behind authorization (cookie based), it's not gonna work at all now :(

@kamilogorek kamilogorek merged commit f5ddf9e into getsentry:master Dec 20, 2017
@kamilogorek
Copy link
Contributor

@RuslanZavacky I asked @HazAT for an opinion, and we can move forward and merge it. I'll try to release it tomorrow after I make sure that there's nothing left that I could add to the next version :)

@RuslanZavacky
Copy link
Contributor Author

🎉 🎉 🎉 thank you, guys!

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