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

Force text content-type #3

Merged
merged 1 commit into from
Sep 3, 2017
Merged

Force text content-type #3

merged 1 commit into from
Sep 3, 2017

Conversation

joehand
Copy link
Collaborator

@joehand joehand commented Aug 31, 2017

It seems to me that Chrome will block all json sendBeacon's right now.

This may break a lot, there is probably a nicer way to do it. Please let me know =).

@yoshuawuyts
Copy link
Member

I was wondering what was going on here, so I chased it down to https://github.com/dxa4481/CORS. Turns out navigator.sendBeacon() now sends an OPTIONS preflight request to the server to figure out if the current client is allowed to write to it.

E.g. probably do something like:

if (req.type === 'OPTIONS') {
  res.setHeader('access-control-allow-origin', '*')
  res.setHeader('access-control-allow-methods', '*')
  res.setHeader('access-control-allow-headers', '*')
  res.setHeader('access-control-allow-credentials', 'true')
}
res.end(200)

And in prod replace the values for the name of your domain, and the values being sent. I think this would be _the right way to do things_™. Going to be adding it to bankai right away!

@joehand
Copy link
Collaborator Author

joehand commented Sep 1, 2017

Did you try this out? The spec says the browser is supposed to send the preflight. But it was my impression that Chrome incorrectly implemented it without preflight checks (and then firefox copied them). From the chrome bug:

Regarding ETA for (A), it turned out to be not a quick work given Chromium's architecture. The CORS code lives in the renderer. There's no reliable way to keep a fetch for sendBeacon alive while waiting for preflight being processed in the browser process. It requires some work.

Search for preflight for a bit more context on that page. Also helpful comment.

It seems a bit broken all around right now unfortunately. That is why I thought using text may be best solution for now (its what google analytics does too).

@yoshuawuyts
Copy link
Member

Woah, no I haven't tried it out yet. I thought I understood the bug correctly after reading it, but guess it's time to make a repro to figure out how to handle it. Thanks for pointing out!

@yoshuawuyts
Copy link
Member

😢 you're completely right. I misread the error, and looks like this is indeed the only way around it. Ughhhhhhhhhhhhhhhhhhhhhh.

@yoshuawuyts yoshuawuyts merged commit 4aa52e1 into choojs:master Sep 3, 2017
@yoshuawuyts
Copy link
Member

v1.0.2

@joehand joehand deleted the patch-1 branch September 3, 2017 19:12
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.

None yet

2 participants