-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add option to set stable or insiders build #20
Conversation
Addresses microsoft#17 Adds an extension options page, stores the value in sync storage, and switches the domain based on the synced value By default, it uses the stable build, since I imagine what most users would prefer
|
||
let preferredVSCodeBuildDomain; | ||
|
||
chrome.storage.sync.get({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is a callback, could it be possible that preferredVSCodeBuildDomain
is undefined or stale at the time that a redirect is invoked? Could we instead read this value at the time of invoking a redirect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the sync get fails (the value isn't found), it's support to use the default value passed in to it. So I don't know what case it wouldn't be set, but I'm not an expert with this API so that could be a problem.
I did consider reading the value each time, but was worried that might cause some delay, since it's an extra I/O check per invocation. But, it's also a small enough value being read that I don't think it'd matter very much.
I'll change this to check per invocation and see if I notice any extra latency. If not, I'll leave that change. If I do notice latency, I'll set preferredVSCodeBuildDomain
to the default so it will always work, even if that value is old
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tired playing with it a little, but unfortunately since this is a network call, it's asynchronous and doesn't seem to play nice with the rest the code. I tried async-ifying some of the code to see if that would help (though I am not well versed with JS's async flow), and that didn't seem to get things to work.
I've updated it to set a default value, so we should always have something valid. Is that satisfactory or would you like me to try something else?
This will ensure that we always have a value, even if it's not the chosen one
This also changes the icon size to be 128x128, as I found larger ones threw errors
Also added an insider icon, so the tool bar button will reflect the settings |
Addresses #17
Adds an extension options page, stores the value in sync storage, and switches the domain based on the synced value
By default, it uses the stable build, since I imagine what most users would prefer