nodejs.org icon indicating copy to clipboard operation
nodejs.org copied to clipboard

enhancement: better detect apple silicon

Open ljharb opened this issue 2 months ago • 15 comments

Description

Use the snippet from https://github.com/faisalman/ua-parser-js/issues/732#issue-2348848266 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

  • https://github.com/faisalman/ua-parser-js/issues/732#issue-2348848266
  • #8324
  • #8328

Check List

  • [x] I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • [x] I have run pnpm format to ensure the code follows the style guide.
  • [x] I have run pnpm test to check if all tests are passing.
  • [ ] I have run pnpm build to check if the website builds without errors.
  • [ ] I've covered new added functionality with unit tests if necessary.

ljharb avatar Nov 05 '25 05:11 ljharb

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
nodejs-org Ready Ready Preview Nov 5, 2025 5:49am

vercel[bot] avatar Nov 05 '25 05:11 vercel[bot]

Codecov Report

:x: Patch coverage is 48.14815% with 14 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 76.64%. Comparing base (30d6c52) to head (c6ed3d5). :white_check_mark: All tests successful. No failed tests found.

Files with missing lines Patch % Lines
apps/site/hooks/react-client/useDetectOS.ts 48.14% 14 Missing :warning:
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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Nov 05 '25 05:11 codecov[bot]

I appreciate the effort, Jordan (don't want to discredit it), but this PR is a strong :-1: for me. Opinions, cc @nodejs/nodejs-website?

ovflowd avatar Nov 05 '25 14:11 ovflowd

I mean the solution is hacky indeed, but at least it works, no? 😅 #8328 still shows x64 for me on Safari on an M1 device

efekrskl avatar Nov 05 '25 16:11 efekrskl

I mean the solution is hacky indeed, but at least it works, no? 😅 #8328 still shows x64 for me on Safari on an M1 device

I'll investigate what's wrong on that solution.

ovflowd avatar Nov 05 '25 16:11 ovflowd

I mean the solution is hacky indeed, but at least it works, no? 😅 #8328 still shows x64 for me on Safari on an M1 device

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

efekrskl avatar Nov 05 '25 16:11 efekrskl

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.

ljharb avatar Nov 05 '25 19:11 ljharb

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.

ovflowd avatar Nov 05 '25 21:11 ovflowd

@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.

ljharb avatar Nov 06 '25 03:11 ljharb

@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.

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

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.

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.

ovflowd avatar Nov 06 '25 05:11 ovflowd

Hey @ljharb I wanted to publicly apologize as after re-reading my own comments here, I happened to notice that I was unnecessarily hostile here and provocative. Whilst these were not my intentions nor I realized the weight of them at the time of writing, I should have done better.

I absolutely understand you're just trying to offer a solution to us, and as I said before I appreciate you and your effort.

Let me take a step back, I still believe that this PR as it stands in a state that is unmergeable, let's try to change that:

  • Can you either verify the license of the code or change the code in a way that is different from the original code so that we don't need to worry too much about it?
  • Could you please verify what exactly these WebGL flags are, what they are for and if they absolutely will work in favor of us for what we need?
  • Could you document sources for where the original ideas came from and how they were verified?
  • Lastly, would you mind adding tests that ensure the happy paths are covered? Since I don't know the real behavior of these flags I don't think that testing unhappy paths would make much sense as these APIs are nevertheless outside of our control.

I do think that @bmuenzenmeyer's PR should be enough, but I'm happy to iterate here if you feel necessary.

Again, I'm really sorry, and I should've done better!

ovflowd avatar Nov 07 '25 18:11 ovflowd

Given that we have an open PR without three blockers, I've decided to close this. If we still see problems with the current implementation post the other PR, we can re-evaluate then

avivkeller avatar Nov 11 '25 16:11 avivkeller

I think closing is premature, since this PR would address the issue more robustly than the open PR, and doesn't block that PR from landing (i think it's already landed?)

ljharb avatar Nov 12 '25 07:11 ljharb

No, I don't think it's premature. We've already emphasized that this approach will not land for a number of reasons, so why should we clutter the issue tracker with it?

If you are adamant that this is the way to go, and we are adamant that it's not, this PR won't go anywhere.

avivkeller avatar Nov 13 '25 11:11 avivkeller

To be clear: my only concern with this approach is the license.

Having this here is not cluttering anything if we are all committed to progress and iteration.

bmuenzenmeyer avatar Nov 13 '25 14:11 bmuenzenmeyer