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: download button pointing to an incorrect binary on Windows arm64 #7240

Merged
merged 2 commits into from
Nov 19, 2024

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 nodejs#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
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

Copy link

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 🔗

Copy link

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 ⏱️

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

@RedYetiDev
Copy link
Member

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

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