betaacid / expo-analytics

Google Analytics integration for use with React Native apps built on Expo
MIT License
288 stars 63 forks source link

FIX: only set user-agent header if not web #63

Closed chunghe closed 4 years ago

chunghe commented 4 years ago

We're using expo-analytics to build web (using expo-web) product. But after launching our product, we've seen significant drop for our users activities. It turns out that on Safari, requests to google analytics all being blocked by the browser. After some digging, User-Agent header will make this CORS request not simple. For safari, it will send an OPTION request to google analytics, google analytics will return 405 error and all the successive requests are blocked.

It's also reasonable for web not sending this header, it always send this header already

截圖 2019-12-05 上午9 29 48
ryanvanderpol commented 4 years ago

@chunghe thanks for this!

Can anyone else verify that they have encountered this issue? I have not seen it, so I want to make sure that if this is an edge case that we are considering the proper parameters.

IgnasA commented 4 years ago

I have a similar issue with android devices. Maybe this pr would solve it.

Network request failed

Stack trace:
  node_modules\react-native\Libraries\vendor\core\whatwg-fetch.js:504:29 in onerror
  node_modules\event-target-shim\lib\event-target.js:172:43 in dispatchEvent
  node_modules\react-native\Libraries\Network\XMLHttpRequest.js:578:29 in setReadyState
  node_modules\react-native\Libraries\Network\XMLHttpRequest.js:392:25 in __didCompleteResponse
  node_modules\react-native\Libraries\vendor\emitter\EventEmitter.js:191:12 in emit
  node_modules\react-native\Libraries\BatchedBridge\MessageQueue.js:349:47 in __callFunction
  node_modules\react-native\Libraries\BatchedBridge\MessageQueue.js:106:26 in <unknown>
  node_modules\react-native\Libraries\BatchedBridge\MessageQueue.js:297:10 in __guard
  node_modules\react-native\Libraries\BatchedBridge\MessageQueue.js:105:17 in callFunctionReturnFlushedQueue
spencerlevitt commented 4 years ago

I just added a PR a few minutes ago — I was experiencing the exact same issue. Rather than taking off the User-Agent header, I added mode: "no-cors" to the fetch request, which might be a better solution than taking off the user-agent header.

ryanvanderpol commented 4 years ago

Thanks for looking into this, @spencerlevitt. I like this approach better than just completely removing the User-Agent. Since this package is not actually intended to be used on the web, have you verified that this change has no adverse effect to use in mobile Expo?

spencerlevitt commented 4 years ago

@ryanvanderpol I just went ahead and tested it again on mobile to make sure, and I'm still getting the "success" console and the page hit sent to GA with the no-cors fix (attaching screenshots). Of course, if you'd like me to, I can make the commit with no-cors if Platform.OS == 'web'. Let me know!

Screen Shot 2020-04-06 at 10 53 27 AM
ryanvanderpol commented 4 years ago

Great, thanks. I think just to be extra safe and not introduce potentially breaking changes, I'd like to see the Platform.OS check in there. Once that's done I'm happy to merge this in and release it.

spencerlevitt commented 4 years ago

@ryanvanderpol Just added PR #66, which only sets the options method to no-cors if Platform.OS == 'web

I tested this again on both mobile and web it's all working well for me

ryanvanderpol commented 4 years ago

This should have been fixed in #68 and released as 1.0.15. Thanks @spencerlevitt and @chunghe for your help on this one! Please give it a try and report back if there are any issues.