Deprecate AccessTokenAdapter.toAuthorizationCallback(string) #279
Conversation
karolis-zukauskas
requested review from
Animagne,
amikoliunas and
robertas-kerpys
as code owners
| public static toAuthorizationCallback(accessToken: AccessToken): AuthorizationCallback { | ||
| const authorization: Authorization = AccessTokenAdapter.toAuthorization(accessToken); | ||
| return async () => authorization; | ||
| /** @deprecated in 5.2. Use {@link toAuthorizationCallback} with a callback parameter instead. */ |
There was a problem hiding this comment.
can we just deprecate the entire class?
There was a problem hiding this comment.
The class is meant to change from the iTwin.js token format to imodels-clients token format. I don't see a reason why it should be deprecated, even if the formats matched.
There was a problem hiding this comment.
why does the imodels-clients have their own format?
every other place in platform its a string, in fact you guys just construct a string from it, eg https://github.com/iTwin/imodels-clients/blob/main/clients/imodels-client-management/src/base/internal/OperationsBase.ts#L125
this class has been a constant source of issues for consumers, and there is no need for this to be a static, it can be a namespace with utility functions if needed
deprecating entire utility class is out of scope for this pr, but want to mention it as we are touching this file right now, and we can add it to the backlog
There was a problem hiding this comment.
imodels-clients packages are designed to work without iTwin.js. Any website that does not depend on a general purpose backend should be able to use these same clients (more imodels-clients-management), they cover full functionality of iModels APIs and allow any way of getting access token to be used.
We are reducing the impacted area when itwinjs-core is changing to this single adapter package. I cannot remember all of the arguments against passing token as a single string right now (likely concerns about extensibility and not being able to add additional properties). But the biggest issue that I can currently see from the code is IModelHost.getAccessToken returning empty string on failure. If imodels-clients didn't validate that the token at least appears valid, it would cause unnecessary noise on iModels API side.
Depracate
AccessTokenAdapter.toAuthorizationCallback(string)to be replaced withAccessTokenAdapter.toAuthorizationCallback(() => Promise<AccessToken>).#278