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

chore(gatsby): convert sanitize-node to typescript #36327

Merged
merged 7 commits into from
Aug 16, 2022

Conversation

Kornil
Copy link
Contributor

@Kornil Kornil commented Aug 5, 2022

Convert sanitize-node to TS.

Related Issues

#21995

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Aug 5, 2022
@marvinjude marvinjude added topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Aug 5, 2022
@@ -20,15 +68,15 @@ const sanitizeNode = (data, isNode = true, path = new Set()) => {
returnData[key] = o
return
}
returnData[key] = sanitizeNode(o, false, path)
returnData[key] = sanitizeNode(o as data, false, path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this loop to a Object.entries(data).forEach? That should allow TS to pick up the right types and we shouldn't need to cast.

Copy link
Contributor Author

@Kornil Kornil Aug 9, 2022

Choose a reason for hiding this comment

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

Object.entries hardcodes the first value o as string, making tests fail (Symbol/string issue, .each avoids this issue).


if (returnData[key] !== o) {
anyFieldChanged = true
}
})

if (anyFieldChanged) {
data = omitUndefined(returnData)
data = omitUndefined(returnData as data) as data
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can type returnData during initialization, we should be able to get away with no casting 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.

returnData is initalised as {} or [].

  • {} is not a valid IGatsbyNode error: Type '{}' is missing the following properties from type 'IGatsbyNode': id, parent, children, internal, fields
  • [] is not a valid GatsbyIterable<IGatsbyNode> error:
    Type 'never[]' is missing the following properties from type 'GatsbyIterable': source, deduplicate, mergeSorted, intersectSorted, deduplicateSorted

I could cast when we define returnData to avoid the double casting here, but we would still need one as omitUndefined returns a Partial<data> (from lodash, pickBy returns a Partial).

I think it's down to personal preference here, I feel the double cast here is more significant than having it when we define returnData but I'm open to change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable enough. Thank you for the detailed response. 😄

@imjoshin
Copy link
Contributor

Code looks good! Got builds kicked off and I'll check back later. 😄

@imjoshin
Copy link
Contributor

Thanks again @Kornil . We appreciate your patience and all of your contributions to Gatsby 💜

@imjoshin imjoshin merged commit d59e7b6 into gatsbyjs:master Aug 16, 2022
@Kornil Kornil deleted the convert-sanitize-node-to-typescript branch August 16, 2022 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants