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

fix: windows download incorrectly selects 32bit on 64bit machine #5025

Closed
wants to merge 4 commits into from

Conversation

styfle
Copy link
Member

@styfle styfle commented Jan 19, 2023

I just installed Windows 11 64 bit and opened Edge to install Node.js and it incorrectly installed the 32 bit version.

User Agent:

Mozilla/5.0 (Windows NT 10.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36 Edg/109.0.1518.55

Also navigator.cpuClass is undefined so we need a new way to detect the the bitness.

Signed-off-by: Steven <steven@ceriously.com>
@ovflowd
Copy link
Member

ovflowd commented Jan 20, 2023

I appreciate the PR, but we're on a code-freeze right now, not to mention all the current "javascript/jquery" code is going to be trashed, as we transition to the codebase from nodejs.dev.

Would you mind opening an issue instead of a PR? So we can work on that when we start migrating the layouts?

Thank you!

@styfle
Copy link
Member Author

styfle commented Jan 20, 2023

I created issue https://github.com/nodejs/nodejs.dev/issues/3162

I also created PR nodejs/nodejs.dev#3163

@nschonni
Copy link
Member

Closing this, as the error is on the .dev site, and not the .org which this PR is against. .org doesn't have this problem. Thanks for creating the issue for the other site!

@nschonni nschonni closed this Jan 20, 2023
@styfle
Copy link
Member Author

styfle commented Jan 20, 2023

@nschonni The .org site does have this problem too

@styfle
Copy link
Member Author

styfle commented Jan 20, 2023

I created #5028 so you don't lose track of the .org bug either.

However, I suggest you reopen my PR here since it is still relevant.

@nschonni
Copy link
Member

No, the download URL comes from .org for both, but only https://nodejs.dev/en/ has this problem, not https://nodejs.org/en/

@styfle
Copy link
Member Author

styfle commented Jan 20, 2023

@nschonni Both have the same issue. Did you try it?

Here's a screenshot with proof:

image

@nschonni
Copy link
Member

It's possible something in the Insider build is broken, I can reproduce your issue on .dev with both Firefox and Edge, but not on .org.

PS: The solution isn't to wipe out the architecture detection and always serve x64

@styfle
Copy link
Member Author

styfle commented Jan 20, 2023

It's possible something in the Insider build is broken

Could be. Or it could point to changes coming in the future where the user agent no longer includes that information.

The solution isn't to wipe out the architecture detection and always serve x64

How about flipping the if statement so that it checks for 32bit first and then falls back to 64bit if the string didn't match? I think its safe to say most Windows installations are 64bit, right?

@nschonni
Copy link
Member

Could be. Or it could point to changes coming in the future where the user agent no longer includes that information.

Definitely possible, but could also just a be a regression in a particular build. Here is the Edge user agent for Win 11 Edge Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36 Edg/109.0.1518.55
The spec says it should be there https://html.spec.whatwg.org/multipage/system-state.html#dom-navigator, but I'm wondering if there is some privacy masking be tried out in the Insider build

How about flipping the if statement so that it checks for 32bit first and then falls back to 64bit if the string didn't match? I think its safe to say most Windows installations are 64bit, right?

If you can find a way to do that, but I think only 64 had indications in the user agent, so that's why it is done this originally.

I think most of the actual x86 windows versions are approaching EOL, so there may be value in visiting the approach as part of the redesign

@styfle
Copy link
Member Author

styfle commented Jan 20, 2023

I investigated some more and found out Windows 11 is only 64 bit, there is no 32 bit version.

Furthermore, there is no way to detect Windows 11 based on the user agent string.

Are you looking for Windows 11 user agents? Surprisingly, they don't exist and never will. Recently, User Agent strings have started to become less important for browser detection online and some browser developers - notably the Chrome and Firefox development teams - have "frozen" the Windows version number in their user agent strings at "Windows 10". So even as new versions of Windows are released, the user agents for those browsers won't reflect this and will continue to report Windows 10.

https://www.whatismybrowser.com/guides/the-latest-user-agent/windows

So then I found the official microsoft docs which say to use client hints or this JS script:

navigator.userAgentData.getHighEntropyValues(["platformVersion"])
 .then(ua => {
   if (navigator.userAgentData.platform === "Windows") {
     const majorPlatformVersion = parseInt(ua.platformVersion.split('.')[0]);
     if (majorPlatformVersion >= 13) {
       console.log("Windows 11 or later");
      }
   }
 });

https://learn.microsoft.com/en-us/microsoft-edge/web-platform/how-to-detect-win11

I confirmed that this works on the insiders windows 11 build because platformVersion is 16.0.0 which is above the 13.

But then I looked at MDN and saw this can be simplified even further to check for bitness.

navigator.userAgentData.getHighEntropyValues(["bitness"])
 .then(ua => {
   console.log(ua.bitness) // "64"
 });

https://developer.mozilla.org/en-US/docs/Web/API/NavigatorUAData/getHighEntropyValues

I think adding that script should solve the problem since it works on my build of windows 11 😄

Thoughts?

@nschonni
Copy link
Member

That might make sense, but I'd suggest keeping the existing checks, and adding as the other ||. Note that the site is currently mostly frozen while they try and merge the layout from the .dev site, but if you fix it there it might come down to this repo eventually

@ovflowd
Copy link
Member

ovflowd commented Jan 20, 2023

I wanted to mention that UA's are being sunset (frozen) (through phasing). It will be tough to predict future OS's User-Agents in the future, as browsers will stop updating them. This is already reflected in Windows 11 by giving less information.

@styfle
Copy link
Member Author

styfle commented Jan 20, 2023

@nschonni @ovflowd I pushed some changes to this branch to use the new API. Can we reopen this PR?

(function () {
  'use strict';
  if (navigator.userAgentData && navigator.userAgentData.getHighEntropyValues) {
    navigator.userAgentData
      .getHighEntropyValues(['bitness'])
      .then(function (ua) {
        initDownloadHyperlink(ua.bitness);
      })
      .catch(function () {
        // Ignore errors since not every browser supports this API
      });
  } else {
    initDownloadHyperlink('unknown');
  }
})();

@ovflowd
Copy link
Member

ovflowd commented Jan 20, 2023

I'm not sure how adopted this API is. What is the caniuse.com of this API?

Anyways, I would highly recommend not making PRs to nodejs.org, as this will all go to waste. This main.js is going to be discarded very soon, so I recommend a PR against nodejs.dev to improve its detection mechanism there :)

@styfle styfle changed the title fix: windows x86 vs x64 cannot be detected fix: windows download incorrectly selects 32bit on 64bit machine Jan 20, 2023
@styfle
Copy link
Member Author

styfle commented Jan 20, 2023

I did create one here nodejs/nodejs.dev#3163

@ovflowd
Copy link
Member

ovflowd commented Jan 20, 2023

Does that PR contain similar changes? (Leveraging the new API?)

@ovflowd ovflowd reopened this Jan 20, 2023
@styfle
Copy link
Member Author

styfle commented Jan 21, 2023

@ovflowd Yep, both PRs are very similar using the same navigator.userAgentData.getHighEntropyValues API.

@styfle
Copy link
Member Author

styfle commented Feb 1, 2023

@nschonni @ovflowd @shanpriyan Anything else we need to land this?

@ovflowd
Copy link
Member

ovflowd commented Feb 1, 2023

@styfle we're on a code freeze for #4991

@ovflowd
Copy link
Member

ovflowd commented Mar 12, 2023

@styfle could you rebase your PR so we can get this merged?

@SEWeiTung
Copy link
Contributor

@styfle : The file is totally changed with the new tech for Nodejs.org. So yours is useless and I've closed your change and re-open a new PR for you.

Any suggestions welcomed!

@SEWeiTung SEWeiTung closed this Mar 14, 2023
@styfle styfle deleted the patch-1 branch March 14, 2023 14:15
SEWeiTung added a commit that referenced this pull request Mar 17, 2023
Refs:

1. #5028.
2. #5025.

Due to the author doesn't have free time to fix this, I'd like to help.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows download incorrectly selects 32bit on 64bit machine
4 participants