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.

Sign up for GitHub

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

Jump to bottom

chore(gatsby): convert component-data-dependencies to typescript #24028

Merged
merged 13 commits into from
May 14, 2020
Merged

chore(gatsby): convert component-data-dependencies to typescript #24028

merged 13 commits into from
May 14, 2020

Conversation

Kornil
Copy link
Contributor

@Kornil Kornil commented May 12, 2020 •
edited
Loading

Convert src/redux/reducers/component-data-dependencies.ts and test file to TypeScript.

There is one change that can possibly affect production
In src/redux/reducers/component-data-dependencies.ts the action CREATE_COMPONENT_DEPENDENCY was referencing a payload.id in line 19, 20, 31, 32. I found the key to not be used in the action itself, so I removed it. Let me know your opinion, I haven't seen it used.

Related Issues

#21995

@Kornil Kornil requested a review from a team as a code owner May 12, 2020 21:29
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 12, 2020
@blainekasten blainekasten mentioned this pull request May 12, 2020
Closed
@madalynrose madalynrose added status: needs core review Currently awaiting review from Core team member and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 12, 2020
@pieh pieh self-assigned this May 13, 2020
@pieh
Copy link
Contributor

pieh commented May 13, 2020

There is one change that can possibly affect production
In src/redux/reducers/component-data-dependencies.ts the action CREATE_COMPONENT_DEPENDENCY was referencing a payload.id in line 19, 20, 31, 32. I found the key to not be used in the action itself, so I removed it. Let me know your opinion, I haven't seen it used.

This should be fine - it seems like legacy code that originally was added in #1503, but feature later on was removed as part of v2 release

@Kornil
Copy link
Contributor Author

Kornil commented May 14, 2020

Thank you for the tough review @pieh! Added all suggestions, and some.

I removed the number type from nodeId and fixed the tests to reflect that. Also removed type casting in tests for optional chaining and removed casting from reducer file for the preferred non-null operator.

@Kornil Kornil requested a review from pieh May 14, 2020 07:55
pieh
pieh reviewed May 14, 2020
View reviewed changes
packages/gatsby/src/redux/reducers/component-data-dependencies.ts
if (state.connections.has(action.payload.connection)) {
existingPaths = state.connections.get(action.payload.connection)
existingPaths = state.connections.get(action.payload.connection)!
Copy link
Contributor

Choose a reason for hiding this comment

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

this is so weird that TS is not smart enough to know that this for sure exists given the check above it, but 🤷‍♂️

pieh
pieh approved these changes May 14, 2020
View reviewed changes
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you so much @Kornil - will just wait for CI tests to run again after one small small change (missed one string | number -> string change after your latest changes, so I went ahead and applied it)

@pieh pieh added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label May 14, 2020
@gatsbybot gatsbybot merged commit 4c79511 into gatsbyjs:master May 14, 2020
@Kornil Kornil deleted the typescript-gatsby-page-data-dependencies branch May 15, 2020 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pieh pieh pieh approved these changes

Assignees

@pieh pieh

Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes status: needs core review Currently awaiting review from Core team member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Kornil @pieh @madalynrose @gatsbybot