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

Add option to set stable or insiders build #20

Merged
merged 3 commits into from
Oct 24, 2022

Conversation

mirdaki
Copy link
Contributor

@mirdaki mirdaki commented Oct 15, 2022

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

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({
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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
@mirdaki
Copy link
Contributor Author

mirdaki commented Oct 22, 2022

Also added an insider icon, so the tool bar button will reflect the settings

@joyceerhl joyceerhl added this to the October 2022 milestone Oct 24, 2022
@joyceerhl joyceerhl merged commit bf7876d into microsoft:main Oct 24, 2022
@mirdaki mirdaki deleted the addStableOption branch October 24, 2022 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants