enhancement: better detect apple silicon #8330
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8330 +/- ##
==========================================
- Coverage 76.72% 76.64% -0.09%
==========================================
Files 118 118
Lines 9805 9831 +26
Branches 335 340 +5
==========================================
+ Hits 7523 7535 +12
- Misses 2280 2294 +14
Partials 2 2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds Apple Silicon detection to improve architecture identification for macOS users when the User-Agent Client Hints API is unavailable or fails. The implementation uses WebGL detection as a fallback mechanism to differentiate between ARM64 (Apple Silicon) and x86 architectures.
- Added
isAppleSilicon()helper function using WebGL-based detection - Updated fallback architecture logic to return 'arm64' for Apple Silicon devices instead of always defaulting to 'x86'
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ljharb
force-pushed
the
better-apple-silicon-check
branch
from
cff6a7a to
faa4c23
Compare
ljharb
force-pushed
the
better-apple-silicon-check
branch
from
faa4c23 to
f51a179
Compare
There was a problem hiding this comment.
Sorry, I'm not a fan of hacky undocumented snippets. Even less a try/catch just because we don't know what could fail + I definitely have no idea what this specific extension is and if it is exclusive to Apple's Sillicone (M-series) or ARM, or what not.
Also there are no tests whatsoever :/
| // we attempt to fallback to what the User Agent indicates | ||
| bitness = uaIndicates64 ? '64' : '32', | ||
| architecture = 'x86', | ||
| architecture = isAppleSilicon() ? 'arm' : 'x86', |
There was a problem hiding this comment.
I'm not a fan of "platform specific" workarounds. + We are literally creating an element on the page, that is breaking the "React" bubble. I'm not a fan of using API's outside of React (window/globals/etc) and we usually only use with Hooks. If you see the only window. API we have is on an explicit hook for watchMedia (https://github.com/search?q=repo%3Anodejs%2Fnodejs.org+window.&type=code)
|
I appreciate the effort, Jordan (don't want to discredit it), but this PR is a strong 👎 for me. Opinions, cc @nodejs/nodejs-website? |
|
I mean the solution is hacky indeed, but at least it works, no? 😅 #8328 still shows |
I'll investigate what's wrong on that solution. |
Yeah, now that it works I'd also say the other solution is a better one. Thanks for jumping in |
|
I can certainly add tests. I verified that it returns true in Safari on my M4 Mac. As for "hacky", using a Chrome-only nonstandard feature is FAR more hacky than using standard WebGL to infer architecture information. |
With all due respect, but, you're absolutely wrong for calling this a Chrome-only non-standard feature (assuming nonstandard as non-specd). It is a spec https://wicg.github.io/ua-client-hints/#navigatoruadata, it's up to the browsers to implement it. Same frigging thing as TC39. The PR you're providing is a hack around a WebGL flag that by no means is documented (I'm not even sure where to look to their docs) to be used for that purpose and could at any point stop working. Not to mention, again, this PR breaks the walled-garden of React, and will cause Next.js to eject on SSR due to this manual Element creation. |
| // taken from https://github.com/faisalman/ua-parser-js/issues/732#issue-2348848266 | ||
| function isAppleSilicon() { | ||
| try { | ||
| // Best guess if the device uses Apple Silicon: https://stackoverflow.com/a/65412357 | ||
| const webglContext = document.createElement('canvas').getContext('webgl'); | ||
| if (webglContext == null) { | ||
| return false; | ||
| } | ||
| const debugInfo = webglContext.getExtension('WEBGL_debug_renderer_info'); | ||
| const renderer = | ||
| (debugInfo && | ||
| webglContext.getParameter(debugInfo.UNMASKED_RENDERER_WEBGL)) || | ||
| ''; | ||
| if (renderer.match(/Apple/) && !renderer.match(/Apple GPU/)) { | ||
| return true; | ||
| } | ||
|
|
||
| if ( | ||
| webglContext | ||
| .getSupportedExtensions() | ||
| ?.includes('WEBGL_compressed_texture_s3tc_srgb') | ||
| ) { | ||
| return true; | ||
| } | ||
| } catch { | ||
| /**/ | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Also, this approach isn't the one the authors of that library ended up going with
| // taken from https://github.com/faisalman/ua-parser-js/issues/732#issue-2348848266 | |
| function isAppleSilicon() { | |
| try { | |
| // Best guess if the device uses Apple Silicon: https://stackoverflow.com/a/65412357 | |
| const webglContext = document.createElement('canvas').getContext('webgl'); | |
| if (webglContext == null) { | |
| return false; | |
| } | |
| const debugInfo = webglContext.getExtension('WEBGL_debug_renderer_info'); | |
| const renderer = | |
| (debugInfo && | |
| webglContext.getParameter(debugInfo.UNMASKED_RENDERER_WEBGL)) || | |
| ''; | |
| if (renderer.match(/Apple/) && !renderer.match(/Apple GPU/)) { | |
| return true; | |
| } | |
| if ( | |
| webglContext | |
| .getSupportedExtensions() | |
| ?.includes('WEBGL_compressed_texture_s3tc_srgb') | |
| ) { | |
| return true; | |
| } | |
| } catch { | |
| /**/ | |
| } | |
| return false; | |
| } | |
| // Ref: https://github.com/faisalman/ua-parser-js/commit/c391d8a73c1a030c24845bc191b021bfb6cba256 | |
| function isAppleSilicon() { | |
| try { | |
| const canvas = document.createElement('canvas'); | |
| const webgl = canvas.getContext('webgl2') || canvas.getContext('webgl') || canvas.getContext('experimental-webgl'); | |
| const debug = webgl.getExtension('WEBGL_debug_renderer_info'); | |
| const renderer = webgl.getParameter(debug.UNMASKED_RENDERER_WEBGL); | |
| if (renderer.match(/apple m\d/i)) { | |
| return true; | |
| } | |
| } catch { | |
| return false; | |
| } | |
| } | |
There was a problem hiding this comment.
sure, i can pull in that approach if it's better.
There was a problem hiding this comment.
more importantly, ua-parser-js is AGPL, and as far as I know the strong copy-left would prohibit MIT usage
|
@ovflowd that link is explicitly to a draft, which means it's not actually standardized yet. To compare to TC39, it'd be like if a feature was shipped prior to stage 3 - relying on it is still inappropriate. If it's incompatible with next.js, that's one thing, but it shouldn't be incompatible with React whatsoever - the functionality just needs to not be invoked on a server. |
That's why MDN marks it as experimental. But so is the API you're using, no? But you do have merit here, it's definitely not a "stage 3 proposal" by any means
Yes this won't be called on React Server. The issue is SSR (not to be confused with RSC/React Server) This will both cause Next.js to complain about initial client render mismatching server render and Next.js to bail SSR generation and fallback to CSR. |
There was a problem hiding this comment.
@ljharb can you double check my OSS licensing concerns here?
| // taken from https://github.com/faisalman/ua-parser-js/issues/732#issue-2348848266 | ||
| function isAppleSilicon() { | ||
| try { | ||
| // Best guess if the device uses Apple Silicon: https://stackoverflow.com/a/65412357 | ||
| const webglContext = document.createElement('canvas').getContext('webgl'); | ||
| if (webglContext == null) { | ||
| return false; | ||
| } | ||
| const debugInfo = webglContext.getExtension('WEBGL_debug_renderer_info'); | ||
| const renderer = | ||
| (debugInfo && | ||
| webglContext.getParameter(debugInfo.UNMASKED_RENDERER_WEBGL)) || | ||
| ''; | ||
| if (renderer.match(/Apple/) && !renderer.match(/Apple GPU/)) { | ||
| return true; | ||
| } | ||
|
|
||
| if ( | ||
| webglContext | ||
| .getSupportedExtensions() | ||
| ?.includes('WEBGL_compressed_texture_s3tc_srgb') | ||
| ) { | ||
| return true; | ||
| } | ||
| } catch { | ||
| /**/ | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
more importantly, ua-parser-js is AGPL, and as far as I know the strong copy-left would prohibit MIT usage
Description
Use the snippet from faisalman/ua-parser-js#732 (comment) to better address #8324. Related to #8328
Validation
Use Safari or Firefox on an M* Mac and see if the download page defaults to arm instead of x64.
Related Issues
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.