-
Notifications
You must be signed in to change notification settings - Fork 635
fix: auto detect system os to download correct target #7824
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
base: master
Are you sure you want to change the base?
fix: auto detect system os to download correct target #7824
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@wleklinskimateusz is attempting to deploy a commit to the GitButler Team on Vercel. A member of the Team first needs to authorize it. |
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.
Thanks so much for getting this started!
There is one potential issue with calling targetDownload.set(undefined)
, but I'd hope that improving the type definition would catch the problem if it is one.
@@ -45,6 +46,33 @@ | |||
function handleClickOutside() { | |||
showSelect = false; | |||
} | |||
|
|||
async function detectDefaultDownload() { |
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.
Should that not be made typesafe and define the return value?
|
||
if (platform.includes('mac')) { | ||
const userAgentData = await navigator.userAgentData?.getHighEntropyValues(['architecture']); | ||
if (userAgentData?.architecture === 'arm' || userAgentData === undefined) { |
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 these terms were swapped I could imagine TS being smart enough to allow doing userAgentData.architecture
.
userAgentData?: { | ||
getHighEntropyValues(hints: string[]): Promise<{ |
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.
It's not impossible that a browser might implement navigator.userAgentData
before getHighEntropyValues
, so we should declare the getHighEntropyValues
method as being optional
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.
We could also type the function like this so we can always access the returned properties:
type Hints = {
readonly brands: { readonly brand: string; readonly version: string }[];
readonly mobile: boolean;
readonly platform: string;
readonly architecture: string;
readonly bitness: string;
readonly formFactor: string[];
readonly model: string;
readonly platformVersion: string;
};
declare global {
// ...
declare interface Navigator {
userAgentData?: {
getHighEntropyValues?: <T extends [keyof Hints, ...(keyof Hints)[]]>(
hints: T
) => Promise<Pick<Hints, T[number]>>;
};
}
}
Usage:

onMount(() => { | ||
detectDefaultDownload().then( | ||
(platformDownload) => platformDownload && targetDownload.set(platformDownload) | ||
); | ||
}); |
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.
We should put the logic to determine the default inside store.ts
so it doesn't get lost if we ever replace this component.
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.
Thank you for the contribution! I've provided some feedback
🧢 Changes
☕️ Reasoning
If you don't want to use experimental browser features - I change it to: for all MacOS devices, default to Apple Silicon - we won't need to extend Navigator interface and it would simplify the logi
If this PR is related to a specific issue, uncomment this section
and link it via the following text:
🎫 Affected issues
Fixes: #7772