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

Migrate DownloadComponents from nodejs.dev #5303

Closed
1 of 8 tasks
ovflowd opened this issue Apr 20, 2023 · 46 comments
Closed
1 of 8 tasks

Migrate DownloadComponents from nodejs.dev #5303

ovflowd opened this issue Apr 20, 2023 · 46 comments
Labels
feature-request Requesting a new Technological Feature to be added to the Website website redesign Issue/PR part of the Node.js Website Redesign

Comments

@ovflowd
Copy link
Member

ovflowd commented Apr 20, 2023

Continuing the migration of components from nodejs.dev that we started here, we should start migrating the DownloadComponents from the nodejs.dev repository.

This issue will keep track of these components and their development progress.

The Migration of each Component

  • Each Component should go under components/Downloads folder on this repository
  • Each Component has its sub-folder (e.g., components/Downloads/Banner)
  • The folder structure should pretty much be the same (CSS components, React, and sub-components), but feel free to make changes if you see deemed
  • Please add some if the Component still needs to get unit tests. At least to ensure the Component is rendering. If the Component has states/CTAs (Buttons/Triggers), please cover them too.
  • The Storybook of each component should allow interaction with all possible states of the Component
  • If the Component depends on Hooks, please migrate them too, with the following caveat:
    • If that component only uses the Hook, move it to the same folder of the Component
    • Otherwise, migrate it to the hooks folder of the repository.
  • If the Component has a Gatsby-specific logic (e.g. Dark Theme Switcher), if it is straightforward enough (e.g. a dark theme switcher plugin for Next.js or it is simple enough to code by ourselves, e.g. a Hook that we manage together with a React Provider) then feel free to do it.
    • Otherwise, if you have questions, please ask them on this Issue or your Draft PR for the Component.

List of Components to Migrate

@ovflowd ovflowd added website redesign Issue/PR part of the Node.js Website Redesign feature-request Requesting a new Technological Feature to be added to the Website labels Apr 20, 2023
@ovflowd ovflowd moved this to 🔖 Ready in Website Redesign Apr 20, 2023
@ovflowd ovflowd pinned this issue Apr 20, 2023
@HinataKah0
Copy link
Contributor

It looks like util/detectOS.ts is used by multiple components (e.g. Hero, and InstallTabs). Should we consider migrating it first before doing the components?

@ovflowd
Copy link
Member Author

ovflowd commented Apr 20, 2023

@HinataKah0 that makes sense! Can you create an issue to migrate both the useDetectOs Hook and detectOs util? (Also it'd be great if InstallTabs used useDetectOs and not detectOs

@ovflowd
Copy link
Member Author

ovflowd commented Apr 20, 2023

BTW @HinataKah0 I moved Hero to #5308

@HinataKah0
Copy link
Contributor

HinataKah0 commented Apr 20, 2023

I think I'll take on migrating the useDetectOs hooks! It looks interesting!
Edit:
Someone else is working on it

@ovflowd ovflowd moved this from 🔖 Ready to 📋 Backlog in Website Redesign Apr 20, 2023
@ovflowd ovflowd moved this from 📋 Backlog to 🔖 Ready in Website Redesign Apr 20, 2023
@HinataKah0
Copy link
Contributor

HinataKah0 commented Apr 23, 2023

I think I'll work on DownloadCards next. I want to try simplifying this logic.

I just realised that NodeReleaseData type is being used in multiple components too (e.g. DownloadHeader, DownloadCards). Shall we consider quickly migrating types/releases.ts first so we can work on the components in parallel?

Since it's very low effort, I have opened a PR. If we feel it's not necessary, feel free to close it.

@AugustinMauroy
Copy link
Member

Hey guys !
I would like to work on DownloadHeader.

Unless someone wants to do it

@ovflowd
Copy link
Member Author

ovflowd commented May 2, 2023

Hey, @HinataKah0 and @AugustinMauroy I'm getting worried with the migration of the Download Components, as the current types might not align with the types we have now.

I'd like to draw your attention to this: https://github.com/nodejs/nodejs.org/blob/main/layouts/DownloadReleasesLayout.tsx#L19

This is what we use for fetching releases on client-side, and https://github.com/nodejs/nodejs.org/blob/main/layouts/DownloadReleasesLayout.tsx#L28 This is the typing of the object. Please keep these in mind types when doing such PRs (https://github.com/nodejs/nodejs.org/pull/5345/files) to consider that we might have different types with the new Components.

My suggestion is to extract this (https://github.com/nodejs/nodejs.org/blob/main/layouts/DownloadReleasesLayout.tsx#L19-L43) into a React Context + Provider (Pretty much to have the data available through the whole App)

And make changes on the mapping of the data (https://github.com/nodejs/nodejs.org/blob/main/layouts/DownloadReleasesLayout.tsx#L29-L38) to fit what the migrated Components need.

I would go to lengths to say that if we have types already on the major/website-redesign branch, we pretty much need to update them to match this.

Please, feel free to work on this (@HinataKah0 if you have time) and let's hold the current open Download Component PRs until we get this Provider and changes to the types merged so that we can adequately ensure that once we start using the migrated Components, we don't need to go through the hassle of updating/fixing them.

@HinataKah0
Copy link
Contributor

Noted, let me take a look into it.

@HinataKah0
Copy link
Contributor

HinataKah0 commented May 3, 2023

I just checked. Yes, I agree to have a Context + Provider.

However, current nodejs.dev's NodeReleaseData is constructed from these 2 APIs' response:

And NodeReleaseData throws away all the dependencies' version (v8, zlib, ...) returned by /dist/index.json.

In DownloadReleasesLayout, it is only fetching data from /dist/index.json.

I think they can share the /dist/index.json fetcher code but not the data structure itself because I don't see many intersections IMO (comparing https://nodejs.org/en/download/releases and https://nodejs.dev/en/download/releases/ where the fields differences become contrast). And we will remove legacy pages anyway (?)

Like you said, we can have a Context + Provider. So we don't need to do Props Drilling since NodeReleaseData is used by:

  • ApiComponent -> VersionSelector
  • Hero
  • DownloadComponent -> DownloadAdditional, DownloadCards, DownloadHeader, DownloadReleases, DownloadTable

I think we can discuss this in a separate issue so we don't pollute this thread since it's specific for components migration.

@ovflowd
Copy link
Member Author

ovflowd commented May 3, 2023

However, current nodejs.dev's NodeReleaseData is constructed from these 2 APIs' response:

No, we should not try to replicate nodejs.dev approach here, we might not even need the release date. If we need, we can use nodevu (the package we are already using on the main branch for fetching the data.

(Check out the main branch to see its usages). The issue is this data is server-side only as nodevu seems to be dependent on a few Node.js-only APIs (cc @bnb as I was unable to run nodevu on client-side Apps).

Pretty much I want to avoid overcomplexing what we have, and if we can avoid making way too many requests and fetching the schedule.json that'd be great.

@HinataKah0

This comment was marked as outdated.

@HinataKah0
Copy link
Contributor

Hi @ovflowd ,

After I thoroughly studied the usages of NodeReleaseData. Actually we need to call both APIs for now.
I assume:

  • We will be adopting new UI for the download pages (including download previous releases page), right? Not sure if we are going to add additional data below
  • We will be calling the APIs in Client Side

All the fields needed by new UI can be fulfilled by /schedule.json alone, except the latest version of each major which will be used to generate the download links only. To get the latest version of each major, we will call /index.json.

Currently nodejs.dev overcomplicates this by assembling them into NodeReleaseData, each instance has duplicated release dates.

Both nodevu (used in current nodejs.org) and @pkgjs/nv (used in current nodejs.dev) produces big outputs and I don't think they can be used in Client Side (we can copy-paste the code...).

Actually /schedule.json is quite small in size +- 770B while /index.json is far bigger +- 8.2KB since it has many things we don't need.

I think we have a few options...

Option 1: Call both APIs as it is and decouple release schedule and release version

Option 2: Create a new JSON file based on /schedule.json + some new fields, then we can sync it periodically when there is a new release (including minor + patch)
We don't need to call /index.json, so we don't need to waste bandwidth. I think this is the cleanest approach.

[
  {
    major: "v18",
    latestVersion: "v18.15.0", // Note that this is added
    start: "2022-04-19",
    ...
  },
  ...
]

@ovflowd
Copy link
Member Author

ovflowd commented May 5, 2023

We will be calling the APIs in Client Side

We should always support client-side, but given we have SSR now with Next.js, I honestly believe leveraging server-side rendering could be good here.

Both nodevu (used in current nodejs.org) and @pkgjs/nv (used in current nodejs.dev) produces big outputs and I don't think they can be used in Client Side (we can copy-paste the code...).

Big outputs aren't the issue here. Regardless of the approach, the requests will have a "big output" due to their nature. That doesn't mean we cannot leverage other techniques such as:

  • static caching of the release data on server side if possible
    • invalidate the cache only during certain moments
  • static caching of the release date on build-time (what we're already doing for most of the pages that need download data)

Actually /schedule.json is quite small in size +- 770B while /index.json is far bigger +- 8.2KB since it has many things we don't need.

I'd disagree, we need the information to a certain degree that is available on both. nodevu for example does honestly a good job by merging both data and providing a good API. You should test nodevu.

To all contents, we could even build a locally-stored JSON file with the dump of nodevu during build-time. Release data doesn't change often, only when there's a new release, so technically we don't need to request this data.

I honestly believe creating a public/static/nodeReleaseData.json at build-time which is simply the output of nodevu, is a good enough approach.

Then we can use SWR to self-consume the JSON file on the client-side and leverage SWR to do the caching on the client-side.

Following this approach, we also reduce the strain on Node.js Infrastructure (Node.js Distribution Server) where the /dist/index.json lives.

So, sadly I'm against both the options you provided. The intent is great, but there are issues, as I mentioned, ranging from over-complexity, unnecessary network calls happening, and strain on the Node.js Infrastructure.

Let me know what you think about the approach I've suggested.

@HinataKah0
Copy link
Contributor

HinataKah0 commented May 6, 2023

Ah, initially I thought we wanted to offload everything (fetching + merging) to client side. Thus, I was confused on how to utilize nodevu in client side. I tried nodevu and it organizes the information neatly.

Thanks for explaining it thoroughly!

About the leveraging server side approach, I am strongly +1 on it.

creating a public/static/nodeReleaseData.json at build-time

+1 on this.

Big outputs aren't the issue here.

nodevu's output alone is 1.4MB (I tried dumping it into a JSON file, IMO 1.4MB is quite big to me 🤔). Actually we can also process the nodevu's output during build time, ignore unnecessary things (for example: we only need the latest version, this behavior is consistent in nodejs.dev and nodejs.org), formatting. This greatly reduces the resulting file size. Time sensitive operations (e.g. check current status) can be deferred to client side.

Summary so far

Build time:
Process & dump nodevu's output into public/static/nodeReleaseData.json

Client side:
Use SWR to fetch & cache
Do time sensitive operations (e.g. check current status)

@ovflowd
Copy link
Member Author

ovflowd commented May 6, 2023

nodevu's output alone is 1.4MB (I tried dumping it into a JSON file, IMO 1.4MB is quite big to me 🤔). Actually we can also process the nodevu's output during build time, ignore unnecessary things (for example: we only need the latest version, this behavior is consistent in nodejs.dev and nodejs.org), formatting. This greatly reduces the resulting file size. Time sensitive operations (e.g. check current status) can be deferred to client side.

Well actually we only need the latest version of each major version. That would already reduce the size by a lot. Could you try that?

@ovflowd
Copy link
Member Author

ovflowd commented May 6, 2023

Do time sensitive operations (e.g. check current status)

Wdym with this?

@HinataKah0
Copy link
Contributor

Well actually we only need the latest version of each major version. That would already reduce the size by a lot. Could you try that?

+1 😄

Do time sensitive operations (e.g. check current status)

Any comparison with current time check. For example: checking if current state is LTS ltsStart <= now.

@AugustinMauroy
Copy link
Member

Have you a proxy ? have you set a proxy ?

@Padmanabh82
Copy link

nah, that gone automatically but now this

npm ERR! code ENOTEMPTY
npm ERR! syscall rename
npm ERR! path /home/pi/nodejs.org/node_modules/ansi-html-community
npm ERR! dest /home/pi/nodejs.org/node_modules/.ansi-html-community-2fKAlG6F
npm ERR! errno -39
npm ERR! ENOTEMPTY: directory not empty, rename '/home/pi/nodejs.org/node_modules/ansi-html-community' -> '/home/pi/nodejs.org/node_modules/.ansi-html-community-2fKAlG6F'

then chatgpt says to delete that module and then try but same error again with other module

@Padmanabh82
Copy link

and i think tourbo will not work in my rpi

 npm install -g turbo

npm ERR! code 1
npm ERR! path /home/pi/.config/nvm/versions/node/v18.16.0/lib/node_modules/turbo
npm ERR! command failed
npm ERR! command sh -c node install.js
npm ERR! /home/pi/.config/nvm/versions/node/v18.16.0/lib/node_modules/turbo/node-platform.js:41
npm ERR!     throw new Error(`Unsupported platform: ${platformKey}`);
npm ERR!           ^
npm ERR! 
npm ERR! Error: Unsupported platform: linux arm LE
npm ERR!     at pkgAndSubpathForCurrentPlatform (/home/pi/.config/nvm/versions/node/v18.16.0/lib/node_modules/turbo/node-platform.js:41:11)
npm ERR!     at checkAndPreparePackage (/home/pi/.config/nvm/versions/node/v18.16.0/lib/node_modules/turbo/install.js:260:28)
npm ERR!     at Object.<anonymous> (/home/pi/.config/nvm/versions/node/v18.16.0/lib/node_modules/turbo/install.js:310:1)
npm ERR!     at Module._compile (node:internal/modules/cjs/loader:1254:14)
npm ERR!     at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
npm ERR!     at Module.load (node:internal/modules/cjs/loader:1117:32)
npm ERR!     at Module._load (node:internal/modules/cjs/loader:958:12)
npm ERR!     at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
npm ERR!     at node:internal/main/run_main_module:23:47
npm ERR! 
npm ERR! Node.js v18.16.0

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/pi/.npm/_logs/2023-05-13T08_52_04_711Z-debug-0.log

@Padmanabh82
Copy link

Padmanabh82 commented May 13, 2023

the socket timeout comes ramdomly

@HinataKah0
Copy link
Contributor

Actually I think you can run without turbo, see our package.json.

Maybe let's not discuss how to setup locally here, because the issues are usually very specific (don't apply to everyone).
Let's keep the discussion here to be about the tasks themselves.
Our Slack is open for anyone to join. 😄

@Padmanabh82
Copy link

but its not letting me do anything, when i replace one tool, other one creates problem. i think binaries created with rust code was not created for arm devices so i think i need to use a old version. may be next 12 or 11 something

@Padmanabh82
Copy link

please remove my name, i wont be able to contribute because of my hardware

@ovflowd
Copy link
Member Author

ovflowd commented May 14, 2023

No worries, @Padmanabh82... We appreciate the effort anyways! You could open an issue on Vercel's Turborepo requesting an ARM binary support in the future!

I wonder why they don't provide ARM support if M1/M2 MacBooks/Macs are ARM 🤔

@AugustinMauroy
Copy link
Member

I have m1 Ship and everything it's good.

@Padmanabh82
Copy link

i was also thinking that, but dont know 😐 and later i also couldn't find the libs for arm SWC

@ovflowd
Copy link
Member Author

ovflowd commented May 15, 2023

Yea, really weird. Would def recommend opening an issue on vercel repository :)

@araujogui
Copy link
Member

Hey, working on DownloadToggle component

@araujogui
Copy link
Member

Working on DownloadReleases component.

@ovflowd
Copy link
Member Author

ovflowd commented Jun 12, 2023

FYI right now #5365 is a blocker for most of the Components, so let's wait for that to get merged before working on new Components.

@ashutosh887
Copy link

Alright @ovflowd
I can see a significant amount of work has already been done in #5365
I'll wait for it to get completed

@ovflowd ovflowd changed the title Migrate DownloadComponents and Hero Component from nodejs.dev Migrate DownloadComponents from nodejs.dev Jun 15, 2023
@HinataKah0
Copy link
Contributor

Good news!
#5365 has been merged.
Please refer to the PR description for how to use Node Releases data.

@ovflowd
Copy link
Member Author

ovflowd commented Jun 17, 2023

FYI DownloadComponents can now be worked on :)

@ovflowd ovflowd unpinned this issue Jul 7, 2023
@ovflowd ovflowd pinned this issue Jul 13, 2023
@ovflowd ovflowd moved this from 🏗 In progress to 📋 Backlog in Website Redesign Jul 27, 2023
@ovflowd
Copy link
Member Author

ovflowd commented Jul 27, 2023

⚠️ Important Update

Due to this update, we're halting all Component development for now until further notice, as the design will significantly diverge, and we do not want to spend time writing Components we don't even know if a counterpart should exist.

Please follow this discussion as we're going to update as soon as we have final designs and a plan for what should be done. Designs and further information will be shared soon.

@ovflowd ovflowd unpinned this issue Jul 30, 2023
@ovflowd
Copy link
Member Author

ovflowd commented Aug 27, 2023

We're closing this issue for the time-being as we're going either to update these issues if needed or create new ones for the development of the new design.

@ovflowd ovflowd closed this as completed Aug 27, 2023
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Website Redesign Aug 27, 2023
@ovflowd ovflowd reopened this Aug 27, 2023
@ovflowd ovflowd closed this as not planned Won't fix, can't repro, duplicate, stale Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Requesting a new Technological Feature to be added to the Website website redesign Issue/PR part of the Node.js Website Redesign
Projects
Archived in project
Development

No branches or pull requests

6 participants