Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wleklinskimateusz
Copy link

🧢 Changes

☕️ Reasoning

  • For Apple if available look for architecture: if something other than arm - change the target to Intel, if arm or architecture not available, stay with Apple Silicon
  • For linux, I've set it to AppImage by default
  • For windows it's set to windows - (BUT - I haven't had a chance to test it on windows, so it would appreciated)

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

Copy link

vercel bot commented Mar 25, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
gitbutler-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 25, 2025 7:54pm

Copy link

vercel bot commented Mar 25, 2025

@wleklinskimateusz is attempting to deploy a commit to the GitButler Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Collaborator

@Byron Byron left a 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() {
Copy link
Collaborator

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) {
Copy link
Collaborator

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.

Comment on lines +14 to +15
userAgentData?: {
getHighEntropyValues(hints: string[]): Promise<{
Copy link
Contributor

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

Copy link
Contributor

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:

Screenshot 2025-03-27 at 11 00 35

Comment on lines +71 to +75
onMount(() => {
detectDefaultDownload().then(
(platformDownload) => platformDownload && targetDownload.set(platformDownload)
);
});
Copy link
Contributor

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.

Copy link
Contributor

@Caleb-T-Owens Caleb-T-Owens left a 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

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.

Auto choose download package on website based on user OS.
3 participants