Deprecate getServicesInfo() in favor of Sep24.info()
#166
Conversation
| * @throws {ServerRequestFailedError} If the server request to fetch information fails. | ||
| */ | ||
| async info( | ||
| shouldRefresh?: boolean, |
There was a problem hiding this comment.
Can the optional value come before the required one? Even though it has a default value, I don't think shouldRefresh can be in the first position and optional while lang is required.
There was a problem hiding this comment.
@aristidesstaffieri thanks for the review!
I think both params are actually optional here, the = this.anchor.language initializer is there to set a default value for an optional param. Typescript won't let me do something like this: lang?: string = this.anchor.language.
The JSDoc also explicits that through the use of brackets syntax like [paramName] which is meant for optional params.
Since they both could be considered as optional we could invert them (like putting lang first), but the downside is that it would be a breaking change for people out there using the existing Sep6.info() function like this Sep6.info(true), they would need to update it to Sep6.info(undefined, true).
Does that make sense?
There was a problem hiding this comment.
Got it yeah that does make sense. I guess the only downside is that users of info now can't leave off shouldRefresh while setting lang but this is probably fine imo.
There was a problem hiding this comment.
IMO lang should come first, as I don't see shouldRefresh being set very often (you generally should be fine with cached info)
There was a problem hiding this comment.
I think the lang param won't be used as much as well since in most cases it'll probably be derived from this.anchor.language.
But I agree that putting lang first does look better than having shouldRefresh first.
I guess we can revert them and make a BREAKING CHANGE note (which in reality I believe won't break for many people out there as it uses the default anchor language as mentioned earlier).
Another alternative yet is using an object as the function input so that the order of the parameters won't really matter. I think this object-input approach generally makes things more robust and scalable as it prevents bugs related to input params out of order and seems easier to maintain.
Something like this:
async info({
lang = this.anchor.language,
shouldRefresh,
}: {
lang?: string,
shouldRefresh?: boolean,
}): Promise<Sep24Info> {
...
How do you guys feel about it?
There was a problem hiding this comment.
I think the lang param won't be used as much as well since in most cases it'll probably be derived from this.anchor.language.
That's a good point, I think you are 100% right
There was a problem hiding this comment.
Another alternative yet is using an object as the function input so that the order of the parameters won't really matter. I think this object-input approach generally makes things more robust and scalable as it prevents bugs related to input params out of order and seems easier to maintain.
+1 on that, but I think it will be breaking, won't it? Maybe a 2.0 change?
There was a problem hiding this comment.
Maybe a 2.0 change?
I like the 2.0 change approach. Maybe we could take this as an opportunity to review and make all SDK functions take objects as arguments. I see our current codebase has a mix of functions taking objects vs taking separate parameters, I think it would be great to establish this code convention on 2.0 and stick with it going forward.
What do you guys think?
We could merge this PR "as is" for 1.7 and plan for the wider change on 2.0.
There was a problem hiding this comment.
Awesome, just created this issue to potentially tackle it in next iteration (sprint): [TS Wallet SDK] Release 2.0 to consolidate object as function input
| * @throws {ServerRequestFailedError} If the server request to fetch information fails. | ||
| */ | ||
| async info( | ||
| shouldRefresh?: boolean, |
There was a problem hiding this comment.
IMO lang should come first, as I don't see shouldRefresh being set very often (you generally should be fine with cached info)
We currently have the below
get infofunctions for the related classes and SEPs:GET /infofor theSep6 serverGET /infofor theSep38 serverGET /infofor theSep24 serverGET /infofor thesame Sep24 serverwhich could beconfusingThe proposal of this PR is to
deprecatebothgetServicesInfo()functions and consolidate them into a singleinfo()function like the below for consistency and simplicity:Sep6.info():hitsGET /infofor theSep6 serverSep38.info(): hitsGET /infofor theSep38 serverSep24.info(): hitsGET /infofor theSep24 serverThis PR also adds support for the
langparameter on Sep6.info() and Sep24.info() as covered by the Sep-6 and Sep-24 specs.