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

Node Releases Data SSR #5438

Closed
HinataKah0 opened this issue Jun 14, 2023 · 7 comments
Closed

Node Releases Data SSR #5438

HinataKah0 opened this issue Jun 14, 2023 · 7 comments
Labels
feature-request (Please use the "Feature Request" type instead) website redesign Issue/PR part of the Node.js Website Redesign

Comments

@HinataKah0
Copy link
Contributor

HinataKah0 commented Jun 14, 2023

TLDR

In this PR, we introduced new data flow for Node releases.

  1. In build time, generate a static JSON file containing all Node releases.
  2. In client side, fetch the generated static JSON file using useSWR hook.
  3. See the PR's description for implementation details...

Now, we are considering to enable SSR / Pre-rendering with data. So, we can harness the powerful useSWR alongside the benefits of Server Side Rendering.
See https://swr.vercel.app/docs/with-nextjs#pre-rendering-with-default-data

TLDR ends :)

Things to consider:

  1. Maybe we are aiming to do something like this
<NodeReleasesContext.Provider value={releases}>
  <SWRConfig value={{ fallback }}> -> fallback if releases is null (it happens during Server Side Rendering)
    {children}
  </SWRConfig>
</NodeReleasesContext.Provider>
  1. Maybe we can consider fetching only once, store it inside memory/RAM rather than reading from the JSON file for every requests made. This will greatly improve performance.

Currently I am also not sure how to do this so it requires exploring the options, and trials & errors.

References:

@HinataKah0 HinataKah0 added website redesign Issue/PR part of the Node.js Website Redesign feature-request (Please use the "Feature Request" type instead) labels Jun 14, 2023
@HinataKah0
Copy link
Contributor Author

I'll be happy to explore further on this
But I'll be away on Fri & Sun this week

@ovflowd ovflowd moved this to 📋 Backlog in Node.js Website Jun 14, 2023
@ovflowd
Copy link
Member

ovflowd commented Jun 14, 2023

Now, we are considering to enable SSR / Pre-rendering with data. So, we can harness the powerful useSWR alongside the benefits of Server Side Rendering.

Thinking about this makes me wonder if we actually need client-side fetching of this data et all. Of course by doing it client-side we're removing this from the actual bundle, but we're then adding extra rendering after the initial rendering and even more data to be fetched beforehand.

I think it really depends on if we're fine with only having "client-side" fetching of this data, as of now, you can clearly see some of the components that require DownloadData taking a little bit to load even on the most optimal network situations.

I'm not sure if we actually add a getServerSideProps to the codebase if Next.js will complain once we try to export it to static mode. (Optimal scenario would be that it just ignores any getServerSideProps.

If that's the case, then we can make the "fallback"/server-side pre-rendering happen only on Vercel Environment, which is still fair, as we mostly will be using SSR Environments. (And it is not a breaking change, just a slightly better behaviour for when an SSR environment is present).

@HinataKah0
Copy link
Contributor Author

HinataKah0 commented Jun 15, 2023

I have the same doubt as well. SSR makes the Client Side fetching redundant (the data hardly changes at all so there's no benefit from re-fetching it).

IMO the benefit from Client Side fetching that I can see is merely offloading the CPU intensive works since we are processing a list of roughly 20 items for every requests (and this list is growing over time). 20 is a small number but it becomes huge at scale. This is of course just a guess currently, I can't see any metric.

My initial thought was to enable it for SEO only (maybe we can check the user agent, we can enable SSR if the user agent comes from Google's bot). This is assuming if we want to keep Client Side fetching.

@ovflowd
Copy link
Member

ovflowd commented Jun 15, 2023

I have the same doubt as well. SSR makes the Client Side fetching redundant (the data hardly changes at all so there's no benefit from re-fetching it).

I think it's more that it adds a pre-rendering aspect into it.

Another alternative is that the JSON file itself is imported during build-time, instead of we fetching it. This would mean that the JSON would be part of the bundle itself, and over time the file can get bigger, but so far, it has only 1.1 kilobytes which is ridiculously small. The XHR request itself and preflight requests take more time than fetching this JSON-

I believe as a follow-up PR, we could try out the following:

  1. You create an empty JSON file on public/node-release-data.json only containing [] which is an empty array.
  2. You commit that initial revision of the file, but any further changes to the file should not be committed (git ignore)
  3. We import the JSON file directly on the NodeReleasesProvider
  4. We remove SWR from the equation
  5. Observe the impact on the bundle (bundle size impact should be minimal)
  6. Compare the behaviour of the initial PR with this one

IMO the benefit from Client Side fetching that I can see is merely offloading the CPU intensive works since we are processing a list of roughly 20 items for every requests (and this list is growing over time). 20 is a small number but it becomes huge at scale. This is of course just a guess currently, I can't see any metric.

This is far from being CPU intensive. Only if it was at the house of millions and even tho we don't do anything fancy with this data.

My initial thought was to enable it for SEO only (maybe we can check the user agent, we can enable SSR if the user agent comes from Google's bot). This is assuming if we want to keep Client Side fetching.

Exactly, with only client-side fetching we have a little of a SEO problem (if at all)

It's also important to mention that CSR was an approach we chose due to the idea that fetching this data and caching it would be better than bundling it. But having that said, since the size of the JSON file is so small, and it would probably take a century for it to be at a size that it really becomes problematic (if we keep the current structure). Then having CSR/SSR is not really needed, and the JSON can be part of the bundle itself.

Which is what we ultimately did with the internationalisation files.

@ovflowd
Copy link
Member

ovflowd commented Jun 18, 2023

@HinataKah0 if you could make a test PR removing SWR and doing the import of the JSON file directly on Provider as I mentioned on the steps above, that'd be great!

@HinataKah0
Copy link
Contributor Author

@ovflowd I believe this isn't the first time we are investigating bundle size...
Previously I rmb we talked about bundle size before adding mui icons as well
I am thinking that we can add bundle analyzer like this
So we can easily visualize... What do you think?

@ovflowd
Copy link
Member

ovflowd commented Jun 19, 2023

@ovflowd I believe this isn't the first time we are investigating bundle size...
Previously I rmb we talked about bundle size before adding mui icons as well
I am thinking that we can add bundle analyzer like this
So we can easily visualize... What do you think?

Feel free to make a concept/use it. Would love to see our bundle broken down in pieces.

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Node.js Website Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request (Please use the "Feature Request" type instead) website redesign Issue/PR part of the Node.js Website Redesign
Projects
Archived in project
Development

No branches or pull requests

2 participants