TokenSource config at runtime via options#256
Conversation
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
lgtm after you address the comments.
MaxHeimbrock
force-pushed
the
max/token-source-config-at-runtime
branch
from
60b1f85 to
e432074
Compare
| case ITokenSourceFixed fixedSource: | ||
| if (options != null) | ||
| Debug.Log("Token source helper has fixed config, fetch options are ignored."); | ||
| return await fixedSource.FetchConnectionDetails(); |
There was a problem hiding this comment.
thought: I think this is a little weird - the TokenSourceHelper sort of sometimes is an ITokenSourceFixed and sometimes an ITokenSourceConfigurable. I don't know if I see an obviously better way to fix this though so - the thing that immediately came to mind was like a TokenSourceHelperFixed / TokenSourceHelperConfigurable type of split but the user shouldn't really have to care about the difference. TokenSourceHelper<Fixed> / TokenSourceHelper<Configurable> has the same problem. So maybe this is just an inevitable downside to this single TokenSourceHelper approach.
question: Just for my own understanding, do you have access to the currently configured subtype of TokenSourceConfig while the user is editing the c# code in unity? Or do all you know is that "the config is a subtype of TokenSourceConfig and that's it". If it's the former, maybe there's something fancy that could be done here.
There was a problem hiding this comment.
Yes, that is the ugliest part of the design right now. I think from a user perspective it is great to have one UI place where you can drag and drop / file select the config you want to use. I do that myself a lot.
My solution was the Debug.Log("Token source helper has fixed config, fetch options are ignored."); but I also forgot to promote it to a Debug.LogWarning. This would make you aware of the footgun.
I feel like the alternatives to split up the TokenSourceHelper into two is creating more UI chaos and the fixed TokenSource config is probably rarely used.
There was a problem hiding this comment.
I did not come up with a better solution right now. I think I will leave it as it is.
There was a problem hiding this comment.
Yep, makes sense. I couldn't come up with something better either so I think this is probably good enough as is.
3 participants
Background
In the first implementation of TokenSource, there was only the option to use a preconfigured config created as a ScriptableObject in the editor. Often you want to create the options at runtime (e.g. set participant name at runtime)
Changes