Skip to content

Conversation

avivkeller
Copy link
Member

This PR does two things (since they are both related to providers):

  1. It switches the downloadSnippets provider to "use cache", this was a very simple change.
  2. It removes the websiteFeeds and blogData providers in favor of two additional utils (feeds and blog, respectively).
    This is because, since the OpenNext integration, fetching blog posts is no longer an expensive task. Now, it's as simple as loading the blog-data.json file, which is already loaded (in next.json.mjs). Thus, this no longer needs to be a generator/provider pair, and can be a utility.

@avivkeller avivkeller requested a review from a team as a code owner October 19, 2025 19:15
@Copilot Copilot AI review requested due to automatic review settings October 19, 2025 19:15
@vercel
Copy link

vercel bot commented Oct 19, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
nodejs-org Ready Ready Preview Oct 19, 2025 7:19pm

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors blog data handling by removing providers in favor of utility functions and enabling caching for download snippets. Since the OpenNext integration, fetching blog posts is no longer expensive as it simply loads the blog-data.json file.

  • Removes websiteFeeds and blogData providers in favor of feeds and blog utilities
  • Switches downloadSnippets provider to use "use cache" directive instead of React's cache()
  • Moves blog data generation logic from generators to scripts directory

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
apps/site/util/feeds.ts Adds feed generation utilities to replace websiteFeeds provider
apps/site/util/blog.ts Adds blog data utilities to replace blogData provider
apps/site/scripts/blog-data/generate.mjs Moves blog data generation logic from generators directory
apps/site/next-data/providers/downloadSnippets.ts Updates to use "use cache" directive
apps/site/types/blog.ts Changes date field from Date to string type
Multiple component files Updates imports to use new utility functions

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@codecov
Copy link

codecov bot commented Oct 19, 2025

Codecov Report

❌ Patch coverage is 66.99507% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.94%. Comparing base (7015095) to head (0b0b049).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
apps/site/util/blog.ts 15.15% 56 Missing ⚠️
apps/site/util/feeds.ts 45.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8243      +/-   ##
==========================================
- Coverage   76.45%   75.94%   -0.52%     
==========================================
  Files         115      115              
  Lines        9643     9718      +75     
  Branches      317      317              
==========================================
+ Hits         7373     7380       +7     
- Misses       2269     2337      +68     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

IDK how I feel this being all moved as an "util" as it is core pieces of the website.... But then we have also snippets, and other pieces that are important also in util. I feel these aren't util and shouldn't live there. That's why "next-data" (Data related to Next.js)

Can we rethink about moving this? It might not be needed at all and just be fine as it is (without "use cache" and react cache)

@ovflowd
Copy link
Member

ovflowd commented Oct 19, 2025

That said, I still believe this could benefit from "use cache" as could the snippet generation as they are expensive paths and generating/building them only once globally makes much more sense.

"use cache" is recommended for anything computationally expensive.

@avivkeller
Copy link
Member Author

  1. The snippet generation is cached
  2. I don't believe this is computationally expensive, it's only sorting a pre-defined JSON file.

@ovflowd
Copy link
Member

ovflowd commented Oct 20, 2025

I still don't see why this should be an "util", also the generator isn't only sorting, it is doing mapping and a lot of other things. I don't see what's the difference of this being moved/refactored within an util folder. There's no advantage here.

@avivkeller
Copy link
Member Author

I don't think it's a "generator". All the other generators are async functions that populate data, this merely filters pre-generated data by category.

To me, filtering seems more of a utility than a generator.

WDYT @nodejs/nodejs-website? Opinions?

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.

2 participants