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 inference-metadata to typescript #24381

Merged

Conversation

Kornil
Copy link
Contributor

@Kornil Kornil commented May 23, 2020

Convert inference-metadata reducer to TypeScript.

Related Issues

#21995

@Kornil Kornil requested a review from a team as a code owner May 23, 2020 09:57
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 23, 2020
state[type] = deleteNode(state[type], { fields: previousFields })
state[type] = addNode(state[type], { fields: node.fields })
state[type] = deleteNode(state[type], { ...node, fields: previousFields })
state[type] = addNode(state[type], { ...node, fields: node.fields })
Copy link
Contributor Author

@Kornil Kornil May 23, 2020

Choose a reason for hiding this comment

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

These 2 changes have the potential to affect production code as we are now sending a full node instead of just fields. I based this on the fact that the action themselves (deleteNode and addNode) are already typed and coded to accept a full node as second parameter.

Please let me know if this change is acceptable or how I can write it better.

Copy link
Contributor

Choose a reason for hiding this comment

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

This feels correct, so it's just a matter of if tests will pass correctly with this change. Thanks for figuring this out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests seem to pass nicely so it should be all good, I just wanted to make sure this change was seen.

@lannonbr lannonbr removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 23, 2020
}
}

module.exports = (
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a named export and update the import reference.

e.g.

export function inferenceMetadataReducer(state, action) { .... }

@Kornil
Copy link
Contributor Author

Kornil commented Jun 6, 2020

Thank you for the review @blainekasten, I've addressed your feedback, feel free to resolve the conversations if you are ok with the new code ✔️

@Kornil Kornil requested a review from blainekasten June 6, 2020 08:13
Copy link
Contributor

@blainekasten blainekasten left a comment

Choose a reason for hiding this comment

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

This looks great to me, let's merge it!

Thank you so much for contributing to our TypeScript refactor! We have more work to do and we would love to have you stay involved in our transition. Please submit more PRs! 💜

@blainekasten blainekasten merged commit 6c99659 into gatsbyjs:master Jun 16, 2020
@Kornil Kornil deleted the typescript-gatsby-inference-metadata branch June 16, 2020 15:36
axe312ger pushed a commit that referenced this pull request Jun 23, 2020
* Convert file & add missing actions

* Address feedback
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.

3 participants