betaacid / expo-analytics

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

Fix: parameters undefined #59

Closed giautm closed 4 years ago

giautm commented 4 years ago

Fix issue: https://github.com/ryanvanderpol/expo-analytics/issues/58

ryanvanderpol commented 4 years ago

@giautm thanks for submitting this! Are there any side effects of this change? It seems unlikely, but what I would notice is that in the original version, the parameters object is updated before every call to the GA API, so if the screen resolution changes, or any of the additionalParameters change, the original parameters property would be stale.

Perhaps a better implementation of this would be to set it in the constructor, as you currently have it, but then update it as it was doing before inside the promiseGetWebViewUserAgentAsync promise?

giautm commented 4 years ago

@ryanvanderpol : with current implement of promiseGetWebViewUserAgentAsync, It just return the old promise with resolved value, no re-calculate step/update parameters. So the parameters object is updated before every call to the GA API was wrong.

If you want that effect, return a function then call it before every call to GA.

https://github.com/ryanvanderpol/expo-analytics/blob/9c2e8224309d8af37457f29fccf6fd7ddc39eddf/analytics.js#L43-L63

try this test in console,

var a = Promise.resolve(1).then(e => {
  console.log('Called',e);
  return e + 1; 
})
a.then(console.log)
a.then(console.log)
ryanvanderpol commented 4 years ago

@giautm @delanebob this has now been merged in and published to NPM. please let me know if you experience any adverse effects from this, and thank you for your help!