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

meta: refactor data generation to server-side only #6137

Merged
merged 6 commits into from
Nov 24, 2023

Conversation

ovflowd
Copy link
Member

@ovflowd ovflowd commented Nov 22, 2023

Description

This PR completely removes the data generation (blogData, nodeReleaseData) from the client-side and completely removes the generation to be done as a pre-build/pre-serve script.

This allows us to generate the data on-demand within Next.js itself and enable HMR of the data if needed.

This removes the need of stashing, unstaging the pesky files from public/ on every commit and checkout.

This completely removes these JSON files (including site navigation, site config, etc) from the client-side and make them a server-side/static-only data. Which completely removes them from the client-side bundle and drastically reduces the bundle size.

Validation

All pages should be working as expected.

@ovflowd ovflowd added the infrastructure Issues/PRs related to the Repository Infra label Nov 22, 2023
@ovflowd ovflowd requested review from a team as code owners November 22, 2023 16:53
Copy link

vercel bot commented Nov 22, 2023

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

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 23, 2023 7:37pm

@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label Nov 22, 2023
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Nov 22, 2023
Copy link
Contributor

github-actions bot commented Nov 22, 2023

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 100 🟢 97 🟢 92 🟢 92 🔗
/en/about 🟢 100 🟢 95 🟠 83 🟢 92 🔗
/en/about/previous-releases 🟢 99 🟢 93 🟢 92 🟢 92 🔗
/en/download 🟢 100 🟢 95 🟢 92 🟢 92 🔗
/en/blog 🟢 99 🟢 95 🟢 92 🟢 92 🔗

Copy link
Contributor

github-actions bot commented Nov 22, 2023

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 94%
90.59% (366/404) 77.77% (91/117) 89.04% (65/73)

Unit Test Report

Tests Skipped Failures Errors Time
71 0 💤 0 ❌ 0 🔥 5.017s ⏱️

@AugustinMauroy
Copy link
Member

great change. And why not using route handler to generate data. And use fetch api to consume data ?

@ovflowd
Copy link
Member Author

ovflowd commented Nov 22, 2023

great change. And why not using route handler to generate data. And use fetch api to consume data ?

Because, not sure you forgot, we need to support static exports.

@ovflowd
Copy link
Member Author

ovflowd commented Nov 22, 2023

Also why the hell would we want to use an API endpoint for static data.

@AugustinMauroy
Copy link
Member

Because, not sure you forgot, we need to support static exports.

yes with static export it's doesn't work

Also why the hell would we want to use an API endpoint for static data.

To reduce custom part and use such as possible nextjs api.

@ovflowd
Copy link
Member Author

ovflowd commented Nov 22, 2023

To reduce custom part and use such as possible nextjs API.

Sorry, but this doesn't make much sense. This static data needs to be available directly at component level. Using an API means this data would be moved to "getStaticProps" or something similar or require swr or fetch within the components, which for server components works, but still unnecessary fetching (even if cached) on the server-side.

It is a path, and would work, but I doubt it is something that makes sense for something that is just config data.

For the blog post data and release data, I could explore this path, so that we don't call this endpoint on every bootstrap of ISR, but still, something that I need to honestly check what would be the best appropriate path here.

@ovflowd
Copy link
Member Author

ovflowd commented Nov 22, 2023

To reduce custom part and use such as possible nextjs API.

Sorry, but this doesn't make much sense. This static data needs to be available directly at component level. Using an API means this data would be moved to "getStaticProps" or something similar or require swr or fetch within the components, which for server components works, but still unnecessary fetching (even if cached) on the server-side.

It is a path, and would work, but I doubt it is something that makes sense for something that is just config data.

For the blog post data and release data, I could explore this path, so that we don't call this endpoint on every bootstrap of ISR, but still, something that I need to honestly check what would be the best appropriate path here.

This is proving complex. Ill meditate about this later.

@AugustinMauroy
Copy link
Member

AugustinMauroy commented Nov 22, 2023

Don't worry, what you've updated is great.

@ovflowd
Copy link
Member Author

ovflowd commented Nov 22, 2023

Don't worry, what you've updated is great.

Done, I think I was able to stretch some brain cells to their limit.

@ovflowd
Copy link
Member Author

ovflowd commented Nov 22, 2023

cc @nodejs/web-infra would love some reviews of people that know their stuff around web

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why route handler is under [local] ? it's not possible to put on app/api/... ?
the rest to support static export is superbly done

@ovflowd
Copy link
Member Author

ovflowd commented Nov 23, 2023

why route handler is under [local] ? it's not possible to put on app/api/... ?

the rest to support static export is superbly done

No. You also forget that nodejs.org/api is a reserved word. It goes to our api docs.

@AugustinMauroy
Copy link
Member

No. You also forget that nodejs.org/api is a reserved word. It goes to our api docs.

true but my real question is why put route handler under I18n

@ovflowd
Copy link
Member Author

ovflowd commented Nov 23, 2023

No. You also forget that nodejs.org/api is a reserved word. It goes to our api docs.

true but my real question is why put route handler under I18n

Everything in our website goes under a /language/ top level route. To prevent the future where some cloudflare settings could block us from using more than /[locale]/ it just made sense.

Also if in the future we translate any of these, they would already be in the right place 😉

Copy link
Collaborator

@bmuenzenmeyer bmuenzenmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my initial review - I'd still like to pull it down locally and run it on Windows

hooks/react-client/useSiteNavigation.tsx Show resolved Hide resolved
next-data/blogData.ts Show resolved Hide resolved
next-data/generators/websiteFeeds.mjs Show resolved Hide resolved
next-data/releaseData.ts Show resolved Hide resolved
next.dynamic.constants.mjs Show resolved Hide resolved
Copy link
Member

@canerakdas canerakdas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great update 🎉 I think this is an update that will make the process more understandable for contributors/newcomers. I left my review notes

app/[locale]/[[...path]]/page.tsx Outdated Show resolved Hide resolved
components/TopNavigation.tsx Outdated Show resolved Hide resolved
hooks/react-server/useSiteNavigation.tsx Outdated Show resolved Hide resolved
next-data/providers/blogData.ts Show resolved Hide resolved
next-data/providers/blogData.ts Show resolved Hide resolved
Copy link
Collaborator

@bmuenzenmeyer bmuenzenmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside from the comments already mentioned - i wanted to say this looks all good on Windows

@ovflowd ovflowd added this pull request to the merge queue Nov 24, 2023
Merged via the queue into main with commit c463a36 Nov 24, 2023
15 checks passed
@ovflowd ovflowd deleted the meta/refactor-data-generation branch November 24, 2023 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Issues/PRs related to the Repository Infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants