Telefonica / webview-bridge

Novum JavaScript Bridge
MIT License
26 stars 7 forks source link

New getTopazValues method #142

Closed aquiros20 closed 4 days ago

aquiros20 commented 1 month ago

From Topaz SDK we need to obtain a new value and make it available via a new bridge method. The value is "syncId" There's already a method that returns a value from Topaz SDK (token) so the behavior should be similar.

The proposed method is as follows: getTopazValues = () => Promise<{syncId: string}>

This proposal includes a more generic name in order to allow it to return other Topaz values and not only the sync id, in case in the future we require more from Topaz sdk, not to fall in the current situation where the existing method (getTopazToken) is pretty specific. But this part is just a suggestion.

UPDATE: After discussion, the method signature will end up being implemented as follows:

getTopazValues = () => Promise<{syncId?: string}>

Which covers the possible response scenarios:

WanaldinoTelefonica commented 1 week ago

Hi @aquiros20

We purpose the following values.

marcoskolodny commented 1 week ago

Why not deprecating getTopazToken method and including the token in the getTopazValues payload?

pladaria commented 1 week ago

Agree witn @marcoskolodny

The new method could be:

const getTopazInfo = (options: {timeout?: number} = {}) => Promise<{token: string, syncId: string}>

And the old one (getTopazToken) would be deprecated and just an alias that returns the same as getTopazInfo:

/** @deprecated **
const getTopazToken = getTopazInfo;

Some questions:

WanaldinoTelefonica commented 6 days ago

That a good approach but sync_id is a value that we only need the SDK to start to fetch and token is an async value that we cannot control how much it would need to be ready. Moving token into this method can make it quite slow.

Answering @pladaria questions:

I would focus this issue on creating the new method to fetch the syncID and maybe we can start an other issue to improve that same method to add more features.

pmartinbTEF commented 6 days ago

I think it is better to maintain two separated calls in order to simplify the error management. @pladaria In the login case the app have a timeout of 5 seconds for obtaining the syncID because the login button is disabled until syncId is obtained. In the case of this new bridge method we have not set any timeout. By now the messages that the app can send to Web when requesting the syncId could be the following:

{"type":"GET_TOPAZ_VALUES","id":"0","payload":{"syncId":"Value"}} /Non empty SyncId/ {"type":"GET_TOPAZ_VALUES","id":"0","payload":{"syncId":""}} /Empty SyncId/ {"type":"GET_TOPAZ_VALUES","id":"0","payload":{}} /Null syncId/ {"type":"ERROR","id":"0","payload":{"code":500,"description":"The operation couldn’t be completed."}} /Error because Topaz SDK couldn't be initialized/

dpastor commented 6 days ago

That a good approach but sync_id is a value that we only need the SDK to start to fetch and token is an async value that we cannot control how much it would need to be ready. Moving token into this method can make it quite slow.

Answering @pladaria questions:

  • I haven't seen anywhere the timeout is used
  • Fields should be optional, that is what the SDK returns
  • Thats a problem if some of the values your fetching are failures and some other don't. In this case we can set as null each value that failed, but you cannot know what happened, or we can add booleans in the model to set if some value failed.

I would focus this issue on creating the new method to fetch the syncID and maybe we can start an other issue to improve that same method to add more features.

With other bridge methods (Specially the ones related to eSim information fetch), we had similar discussions about having one or more bridge methods. IMO more granular bridge methods are always easier to implement, simpler, faster, and can have specific error responses for the specific information being requested. Also, as @WanaldinoTelefonica mentioned, if in this case, information is available in different moments, and error management could be complex as @pmartinbTEF mentions, so, these not be grouped, no?

pladaria commented 6 days ago

Ok, let's go with a new method.

@aquiros20, please update the ticket description setting the sync_id as optional/nullable

@pmartinbTEF regarding this:

{"type":"GET_TOPAZ_VALUES","id":"0","payload":{"syncId":""}} /Empty SyncId/

what's the difference between an empty syncId and a null one? Can we unify both cases as payload: {}?

WanaldinoTelefonica commented 5 days ago

I don't know if an empty syncID is the same as a nul one but we can agree that both are not an ID so we can omit from the payload if everyone is agree.

pladaria commented 5 days ago

ok, LGTM @aquiros20, please update the issue description with the final proposal that should be implemented

aquiros20 commented 5 days ago

ok, LGTM @aquiros20, please update the issue description with the final proposal that should be implemented

Done.