Fix/#155264394 added missing on error methods #14
Conversation
- added onError methods for TaplyticsNewSessionListener and SessionInfoRetrievedListener to the react wrapper
- Updated build.gradle to match latest SDK version
| @@ -243,6 +246,9 @@ public void onNewSession() { | |||
| params.putBoolean("value", true); | |||
| sendEvent("newSession", params); | |||
| } | |||
| public void onError() { | |||
|
|
|||
There was a problem hiding this comment.
do we need to send an event with the value of false here?
There was a problem hiding this comment.
it should also be overriding onError()...
@Arpit-Patel you can open this file in Android studio and it will show you its errors
There was a problem hiding this comment.
I don't see any errors in studio with it @VicV.
@jonathannorris I will add that
There was a problem hiding this comment.
@VicV why doesn't this one have a promise like the other ones?
There was a problem hiding this comment.
@jonathannorris there is no promise because its just setting a listener.
| @@ -283,6 +289,9 @@ public void sessionInfoRetrieved(HashMap hashMap) { | |||
|
|
|||
| callback.resolve(resultData); | |||
| } | |||
| public void onError(HashMap hashMap) { | |||
| callback.resolve(null); | |||
There was a problem hiding this comment.
should we reject the promise?
There was a problem hiding this comment.
@jonathannorris not sure what rejecting the promise does inside of the react package. Need to ask @ajwootto about that.
@Arpit-Patel This should be resolved with an empty hashmap I believe
There was a problem hiding this comment.
well rejecting the promise is basically the same as an error response.
There was a problem hiding this comment.
yeah I think this should probably reject with an error message
| @@ -232,6 +232,9 @@ public void startNewSession(final Promise callback) { | |||
| public void onNewSession() { | |||
| callback.resolve(null); | |||
| } | |||
| public void onError() { | |||
| callback.resolve(null); | |||
There was a problem hiding this comment.
should we reject the promise?
- Changed value to event.value since value is not declared - Removed promise from _setTaplyticsNewSessionListener, since this is not explicitly called by user
| public void onError() { | ||
| WritableMap params = Arguments.createMap(); | ||
| params.putBoolean("value", false); | ||
| sendEvent("newSession", params); |
There was a problem hiding this comment.
Does it make sense to still emit this event on error? What if people are using the listener but ignoring the value?
There was a problem hiding this comment.
@ajwootto yeah you're right that doesn't make sense.
Would it be better to just use a promise and then use a then and catch in the index.js which also resolves and rejects? So that the user can know if setting failed or not.
There was a problem hiding this comment.
I dont think so because this is technically a "listener" which means it can be called many times at any time by the SDK.
There was a problem hiding this comment.
One thing you could potentially do is change the DeviceEmitter listener function on the JS side to take two parameters, a "newSession" callback and an "onError" callback
| @@ -77,7 +77,7 @@ Taplytics.setTaplyticsNewSessionListener = (listener) => { | |||
| Taplytics._setTaplyticsNewSessionListener() | |||
There was a problem hiding this comment.
could change this to take two "listeners" representing success and failure
There was a problem hiding this comment.
Actually I don't think its that useful anymore to even do anything on error cause setTaplyticsNewSessionListener on the sdk side is a very trivial operation. The onError call is really on useful when you're calling startNewSession, since many things can potentially go wrong.
* Refactored SDK to be more modular * Removed TaplyticsProvider imports * Removed unused and undocumented native methods * Added _newSessionCallback native method and exported it as part of `setTaplyticsNewSessionListener` method * Changed newSyncObject to take in NSDictionary * Added getVariables and registerVaraiblesChangedListener methods and removed lodash and only added lodash.clonedeep * Add library to gitignore and prettierignore * Added extra line on ignore files * Added push method types and cleaned up the push methods * Removed old index files and added push types to index * Fixed typo in console.error * Fixed a bug caused by the event listeners not being initialized before they are triggered * Export getRunningExperimentsAndVariations * Reformatted android module, and fixed crash for newSyncObject
No description provided.