Skip to content

Conversation

@wojtekmaj
Copy link
Contributor

@wojtekmaj wojtekmaj commented Nov 19, 2024

Description

This PR changes the way DownloadButton calculates bitness which gets passed to getNodeDownloadUrl.

Previously, this value was passed directly from useDetectOS(), leading to the issue mentioned in the original issue.

After this change, bitness is calculated similarly to how it's done in BitnessDropdown, so: by passing architecture and bitness to getUserBitnessByArchitecture:

const BitnessDropdown: FC = () => {
const { bitness: userBitness, architecture: userArchitecture } =
useDetectOS();
const { bitness, os, release, setBitness } = useContext(ReleaseContext);
const t = useTranslations();
// we also reset the bitness when the OS changes, because different OSs have
// different bitnesses available
useEffect(() => {
setBitness(getUserBitnessByArchitecture(userArchitecture, userBitness));
// we shouldn't update the effect on setter state change
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [userArchitecture, userBitness]);

Validation

See #7239 for reproduction steps.

I've validated the fix by navigating to Vercel preview (linked by a bot below) and verifying the binary link (see bottom left corner):

image

Related Issues

Closes #7239

Check List

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

Closes #7239 

This PR changes the way DownloadButton calculates `bitness` which gets passed to `getNodeDownloadUrl`. 

Previously, this value was passed directly from `useDetectOS()` without any further processing, leading to the issue mentioned above.

After this change, `bitness` is calculated similarly to how it's done in `BitnessDropdown`, so: by passing `architecture` and `bitness` to `getUserBitnessByArchitecture`:

https://github.com/nodejs/nodejs.org/blob/c5345f551ea545cf9d04017204b17f3099940ada/apps/site/components/Downloads/Release/BitnessDropdown.tsx#L16-L29

Signed-off-by: Wojciech Maj <[email protected]>
@wojtekmaj wojtekmaj requested a review from a team as a code owner November 19, 2024 12:06
@vercel
Copy link

vercel bot commented Nov 19, 2024

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

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Nov 19, 2024 8:27pm

@github-actions
Copy link
Contributor

github-actions bot commented Nov 19, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟠 84 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 100 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 92 🔗

@github-actions
Copy link
Contributor

github-actions bot commented Nov 19, 2024

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 92%
90.66% (631/696) 72.58% (188/259) 94.48% (120/127)

Unit Test Report

Tests Skipped Failures Errors Time
137 0 💤 0 ❌ 0 🔥 5.377s ⏱️

@avivkeller
Copy link
Member

Error: //#prettier: command (/home/runner/work/nodejs.org/nodejs.org/) /opt/hostedtoolcache/node/20.18.0/x64/bin/npm run prettier exited (1)

Is this reproducible locally? If not, I'll restart the CI to see if it was a flake?

@AugustinMauroy
Copy link
Member

Is this reproducible locally? If not, I'll restart the CI to see if it was a flake?

No there are no issue with prettier. The code format is just not right.

@wojtekmaj could you do a npm run format to format the code and have a green CI

@wojtekmaj wojtekmaj changed the title Fix Download button pointing to an incorrect binary on Windows arm64 fix: download button pointing to an incorrect binary on Windows arm64 Nov 19, 2024
@wojtekmaj
Copy link
Contributor Author

Ah, sorry! I did that PR entirely on the go :D

@avivkeller
Copy link
Member

No worries! Once all the checks pass, since this has approvals, I've added it to the merge queue

@avivkeller avivkeller added this pull request to the merge queue Nov 19, 2024
Merged via the queue into nodejs:main with commit 6be3b25 Nov 19, 2024
14 of 15 checks passed
@wojtekmaj wojtekmaj deleted the patch-1 branch November 19, 2024 20:53
bmuenzenmeyer pushed a commit that referenced this pull request Nov 21, 2024
…#7240)

* Fix Download button pointing to an incorrect binary on Windows arm64

Closes #7239 

This PR changes the way DownloadButton calculates `bitness` which gets passed to `getNodeDownloadUrl`. 

Previously, this value was passed directly from `useDetectOS()` without any further processing, leading to the issue mentioned above.

After this change, `bitness` is calculated similarly to how it's done in `BitnessDropdown`, so: by passing `architecture` and `bitness` to `getUserBitnessByArchitecture`:

https://github.com/nodejs/nodejs.org/blob/c5345f551ea545cf9d04017204b17f3099940ada/apps/site/components/Downloads/Release/BitnessDropdown.tsx#L16-L29

Signed-off-by: Wojciech Maj <[email protected]>

* Fix formatting

---------

Signed-off-by: Wojciech Maj <[email protected]>
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.

Incorrect download suggested on Windows arm64 machines

4 participants