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

ref: Quick Refactor to Parse the Whole Response of Converted Hooks #2208

Merged

Conversation

nicholas-codecov
Copy link
Contributor

Description

This quick refactor updates hooks that have already been converted over to use the new repository field to slightly adjust the return value as well as parse the whole response rather then a small portion.

Notable Changes

  • Update useCommitHeaderData
  • Update useCommitPageData
  • Update usePullHeadData
  • Update usePullPageData

@netlify
Copy link

netlify bot commented Aug 15, 2023

Deploy Preview for gazebo-staging ready!

Name Link
🔨 Latest commit 321a679
🔍 Latest deploy log https://app.netlify.com/sites/gazebo-staging/deploys/64f8613308b63b0008dcad4d
😎 Deploy Preview https://deploy-preview-2208--gazebo-staging.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov-staging
Copy link

codecov-staging bot commented Aug 15, 2023

Codecov Report

Merging #2208 (321a679) into main (7627391) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2208   +/-   ##
=======================================
  Coverage   97.94%   97.94%           
=======================================
  Files         698      698           
  Lines        7926     7929    +3     
  Branches     1887     1892    +5     
=======================================
+ Hits         7763     7766    +3     
  Misses        161      161           
  Partials        2        2           
Files Changed Coverage
...mitDetailPage/Header/hooks/useCommitHeaderData.tsx 100.00%
...pages/CommitDetailPage/hooks/useCommitPageData.tsx 100.00%
...s/PullRequestPage/Header/hooks/usePullHeadData.tsx 100.00%
...rc/pages/PullRequestPage/hooks/usePullPageData.tsx 100.00%

Continue to review full report in Codecov by Sentry.

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

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #2208 (321a679) into main (7627391) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2208   +/-   ##
=====================================
  Coverage   97.94   97.94           
=====================================
  Files        698     698           
  Lines       7926    7929    +3     
  Branches    1887    1892    +5     
=====================================
+ Hits        7763    7766    +3     
  Misses       161     161           
  Partials       2       2           
Files Changed Coverage Δ
...mitDetailPage/Header/hooks/useCommitHeaderData.tsx 100.00% <100.00%> (ø)
...pages/CommitDetailPage/hooks/useCommitPageData.tsx 100.00% <100.00%> (ø)
...s/PullRequestPage/Header/hooks/usePullHeadData.tsx 100.00% <100.00%> (ø)
...rc/pages/PullRequestPage/hooks/usePullPageData.tsx 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

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

Copy link
Contributor

@RulaKhaled RulaKhaled left a comment

Choose a reason for hiding this comment

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

two quick comments

@@ -132,7 +138,7 @@ export const useCommitHeaderData = ({
}

return {
commit: data?.repository?.commit,
commit: data?.owner?.repository?.commit ?? null,
Copy link
Contributor

Choose a reason for hiding this comment

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

so just so i understand this better, we can't return undefined here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because of the way that GraphQL works. If you request a field it will either return the value or null it will never return undefined so we don't wan't the TS value to reflect something it could never be. Which if we set to undefined here it won't pass the type validation that zod runs and will error out.

])
.nullable(),
})
.nullable(),
Copy link
Contributor

Choose a reason for hiding this comment

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

do we plan to take out the queries to their own files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any benefit as to removing the queries as when they're only used in a single location. I also think it's somewhat of a bad practice in these instances to move a string to it's own file.

@nicholas-codecov nicholas-codecov merged commit 0228a79 into main Sep 6, 2023
15 checks passed
@nicholas-codecov nicholas-codecov deleted the gh-eng-355-quick-refactor-to-parse-the-whole-response branch September 6, 2023 11:33
RulaKhaled pushed a commit that referenced this pull request Sep 6, 2023
…2208)

Quick refactor to parse the whole response over just smaller portions.
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.

None yet

2 participants