chore: refactor the generateBlogData function (again) (#7607)
#7618
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
apps/site/next-data/generators/blogData.mjs:70
- [nitpick] Consider renaming 'rawFrontmatter' to 'frontmatterContent' to more clearly indicate that it stores the accumulated Markdown frontmatter text.
let rawFrontmatter = '';
apps/site/next-data/generators/blogData.mjs:60
- Consider adding unit tests to verify that the concurrent file processing logic (including frontmatter extraction and category aggregation) handles typical and edge-case scenarios correctly.
const posts = await Promise.all(
github-actions
bot
removed
the
github_actions:pull-request
|
Lighthouse Results
|
|
i appreciate the extra effort in testing this, but i'd really like if we could cover this scenario with a test - either unit or lighthouse routes and #7395 |
Unit Test Coverage Report
|
yeah I totally agree with that 👍 I can of course add some unit tests if you want 🙂 However they would have not caught the regression introduced by my changes anyways since those were due to the Next.js build output 😕 , a full e2e test would be the only thing that would have prevented that |
AFAIK thats what one way #7395 could work - run playwright tests against the vercel preview |
|
I'm also sorry for not double checking these myself 🤦 -- I assumed it was working and did not even verify the preview. Because the code made sense. |
| @@ -39,12 +34,6 @@ const getFrontMatter = (filename, source) => { | |||
| // all = (all blog posts), publish year and the actual blog category | |||
| const categories = [category, `year-${publishYear}`, 'all']; | |||
|
|
|||
| // we add the year to the categories set | |||
There was a problem hiding this comment.
Why we removing these categories? 👀
There was a problem hiding this comment.
nono, these are still being added anyways to the blogCategories array, I'm just doing it from the categories array here: https://github.com/nodejs/nodejs.org/pull/7618/files/7004629e8ff0dfcdc7a57109d5132fe34ef301c3#diff-5ddcf76b865830c001627fef7abd06107b3eafeb210d21737305db2416a279beR95-R98
No problem, it also worked on the first page of the blog... so unless you started clicking around it kinda seemed like everything was ok even in the preview 😓
The code sure made sense! it indeed worked perfectly with |
I'm a big fan of testing I'd love to help out with that 🙂, anyways it sounds like a bit of a bad timing to add playwright tests right now given a potential migration onto Cloudflare 😓 (#7383) (or the perfect timing, depending on how you look at it 😅) |
Hey @bmuenzenmeyer 🙂 I've added some unit tests for the |
dario-piotrowicz
force-pushed
the
dario/refactor-generateBlogData
branch
from
06b8c3c to
eacdf5a
Compare
github-actions
bot
removed
the
github_actions:pull-request
Description
Reintroducing the refactoring I added in #7607 that actually broke production ( 🙈 ) and I had to quickly revert in #7617
What went wrong / what I changed from my previous PR
Basically with my previous code the Next.js compiler would move code around breaking things, this is the code it generated for my previous iteration of the file:
The problem being that
f(which is the minified version ofblogCategories) gets spread in the result before theawait Promise.allthat actually side-effectfully populates itIn this PR I am then also moving
blogCategoriesinsidegenerateBlogData, this is arguably cleaner than the previous iteration anyways (in my opinion) and it seems to help the Next.js compiler not to move things around, no longer introducing the issue(as you can see now the
await Promise.allis correctly run before the return statement)Validation
I've manually tested this both in
next devandnext build + next start(both this time! 🙈)Related Issues
Check List
npm run formatto ensure the code follows the style guide.npm run testto check if all tests are passing.npx turbo buildto check if the website builds without errors.