Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

refactor: updating gatsby node file#1662

Merged
manishprivet merged 5 commits into
nodejs:mainfrom
lancemccluskey:refactoring-gatsby-node
Aug 5, 2021
Merged

refactor: updating gatsby node file#1662
manishprivet merged 5 commits into
nodejs:mainfrom
lancemccluskey:refactoring-gatsby-node

Conversation

@lancemccluskey

Copy link
Copy Markdown
Contributor

Description

Refactor parts of gatsby-node into separate functions in separate files for readability.

Related Issues

Fixes #1649

async function getBannersData() {
try {
const siteResponse = await fetch(
'https://raw.githubusercontent.com/nodejs/nodejs.org/master/locale/en/site.json'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Couldnt get the mocks to work for this call and the calls in getNodeReleasesData. Idk what i could be doing wrong though. I ended up deleting the tests. Maybe they can be a future PR

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #1662 (0321871) into main (be7cd32) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1662   +/-   ##
=======================================
  Coverage   90.49%   90.49%           
=======================================
  Files          72       72           
  Lines         768      768           
  Branches      219      220    +1     
=======================================
  Hits          695      695           
  Misses         72       72           
  Partials        1        1           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be7cd32...0321871. Read the comment docs.

@manishprivet

Copy link
Copy Markdown
Member

I think you should wait for #1650 to be approved and merged before working any more on this, since major changes will be required here after that

@manishprivet manishprivet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have highlighted the major changes that needed to be done when moving from MarkdownRemark to Mdx.

Comment thread gatsby-node.js Outdated
Comment thread util-node/createPagesQuery.js Outdated
Comment thread util-node/createPagesQuery.js Outdated
Comment thread gatsby-node.js Outdated
@lancemccluskey

Copy link
Copy Markdown
Contributor Author

@manishprivet I fixed all the merge conflicts. Everything should be working correctly now

@rodion-arr rodion-arr added the create-preview Generate preview on staging.nodejs.dev label Aug 3, 2021
@github-actions github-actions Bot removed the create-preview Generate preview on staging.nodejs.dev label Aug 3, 2021
@github-actions

github-actions Bot commented Aug 3, 2021

Copy link
Copy Markdown

Please find a preview at: https://staging.nodejs.dev/1662/

@manishprivet

Copy link
Copy Markdown
Member

LGTM 👍

@rodion-arr rodion-arr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've checked the fetch mock for missing tests and indeed it's not working there. Very interesting case, I'd like to dig deeper after this PR merged

@manishprivet manishprivet merged commit 748799a into nodejs:main Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor / Rewrite gatsby-node.js

5 participants