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

For each call to fetch, set the User-Agent header #1234

Closed

Conversation

shawwn
Copy link
Contributor

@shawwn shawwn commented Jun 26, 2019

Closes #1230

For each call to fetch in src/DOM.js, this PR sets the User-Agent header to window.navigator.userAgent, where window is defined as this.ownerDocument.defaultView.

This seems to be the correct way to get the window associated with the dom element in a multi-window environment, and window.navigator.userAgent seems to be the correct value to pass.

@avaer
Copy link
Member

avaer commented Jun 26, 2019

Per avaer/window-fetch#2, I don't think adding manual options everywhere is the right resolution.

@shawwn
Copy link
Contributor Author

shawwn commented Jun 26, 2019

Suppose the intercept feature is done.

At that point, you'd still need to decide what User-Agent value to pass to fetch. And that decision would presumably be made somewhere other than inside the DOM element, just before the call to fetch.

In a multi-window multi-worker environment, it's not possible to know what the value of User-Agent should be for any arbitrary DOM element. The correct User-Agent value is window.navigator.userAgent. However, given DOM element X and DOM element Y, then X and Y might have completely different values for this.ownerDocument (and hence, different values for their corresponding window.navigator.userAgent).

Suppose User-Agent is a static unchanging value, knowable at startup time. Then in that case, simply setting something like

const _fetch = fetch;
fetch = (url, opts, ...args) => {
  let h = (opts && opts.headers) ? opts.headers : new Headers();
  if (!(h instanceof Headers)) { h = new Headers(h); }
  if (!h.has('User-Agent')) {
   h.set('User-Agent', `User-Agent: ${ExokitUserAgent}`);
  }
  return _fetch(url, {... (opts || {}), headers: h}, ...args);
}

at the top of src/DOM.js would give you the equivalent functionality as the intercept API. But it's ugly compared to just passing in a value.

@avaer
Copy link
Member

avaer commented Jun 26, 2019

The purpose of interception is to generalize the above case into a central control point.

That includes:

  • Different fetch callpoints
  • XMLHttpRequest
  • User fetch
  • Various Workers

@RangerMauve
Copy link
Collaborator

Yeah, I agree with @modulesio. This is a really great idea and we should be setting the header, but it'd be better to do it inside the fetch implementation rather than in the code invoking fetch.

@shawwn
Copy link
Contributor Author

shawwn commented Jun 26, 2019

Closing in favor of #1237

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.

exokit DOM doesn't support google analytics
3 participants