Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow getting all tokens #77

Closed
wants to merge 5 commits into from
Closed

Conversation

sdFSDFSADF
Copy link

rewrite of getMsaTokens() but returns full ret instead

useful for those who want everything

lmk if docs need to be written too its pretty much the same as getMsaToken()

rewrite of getMsaTokens() but returns full ret instead

useful for those who want everything

lmk if docs need to be written too its pretty much the same as getMsaToken()
@sdFSDFSADF
Copy link
Author

when testing it returns
{ accessToken: '...' }

still testing

getMsTokens() is not compatible with cache.
@sdFSDFSADF
Copy link
Author

fixed. removed cache from function due to the functions nature and use

@sdFSDFSADF
Copy link
Author

never mind, still returning just accesstoken. looking into livetokenmanager.js

created function fullAuthDeviceCode and made it return full response
change to fullAuthDeviceCode()
@sdFSDFSADF
Copy link
Author

should be fully fixed

@extremeheat
Copy link
Member

First, per the don't repeat yourself principle, it's best in code to not repeat large pieces of code when adding new functionality. What you should be doing is adding abstraction. In this case, via updating (and renaming) the current function to expand the data it returns, then adding a new function in the same name as the old one that calls the lower function and returns what's expected by the current API.

Since the TokenManager/* files are internal, it's ok to ignore compatibility there, so the existing code can be changed as needed.

Also, returning arbitrary data from a server is not good. This would need to be added to the doc yes, along with explicitly what data we're providing. As we have to keep that stable if it changes.

@rom1504
Copy link
Member

rom1504 commented Dec 17, 2023

ci is failing

@rom1504
Copy link
Member

rom1504 commented Mar 17, 2024

please re open if you want to finish it

@rom1504 rom1504 closed this Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants