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-cli): migrate redux folder to typescript #22292

Merged

Conversation

Kornil
Copy link
Contributor

@Kornil Kornil commented Mar 15, 2020

Description

Migrate /redux folder in gatsby-cli to typescript.

Related Issues

#21995

There are a couple of is one anys that I could not type better. Let me know if you have any feedback.

@Kornil Kornil requested a review from a team as a code owner March 15, 2020 09:39
@MichaelDeBoey MichaelDeBoey changed the title chore(cli): migrate redux folder to typescript chore(gatsby-cli): migrate redux folder to typescript Mar 15, 2020
@Kornil
Copy link
Contributor Author

Kornil commented Mar 16, 2020

I'm not sure how I should handle module.exports syntax. Especially on /redux/index.ts. Any guidance? Test fails are related to that export.

@MichaelDeBoey
Copy link
Contributor

@Kornil you can just use export default

@Kornil
Copy link
Contributor Author

Kornil commented Mar 16, 2020

@Kornil you can just use export default

@MichaelDeBoey so can I increase the scope of the PR to fix all the affected files as well?
Files like
packages/gatsby-cli/src/reporter/loggers/ink/context.js (line 3)
packages/gatsby-cli/src/reporter/loggers/ipc/index.js (line 1)

Import by using the non-default export syntax, so they won't be compatible anymore I believe.

dispatchAction | { type: Actions; payload: Partial<IActivity> }
>

export interface IActionTypes {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the need for this to be reusable (in the sense of being an export in a common file). Could you just transfer these types to exist on the functions themselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to export them types manually instead of an interface? I put them in a different file cause it grew quite a lot.

@Kornil
Copy link
Contributor Author

Kornil commented Mar 16, 2020

Thank you @blainekasten for the tough review, I've commented with an 👍 on each of your comments that I've addressed.

Return types for actions are now fully typed with a clean interface for both the type and payload.

Only one I left out as I feel it is easier to have an object and interface combo vs key -> type for each one.

Few things to note:

  • Still not sure how to better type this, google is not helpful at all here.
export default {
  ...bindActionCreators<typeof actions, any>(actions, dispatch),
}
  • One Partial is left for the update action, as we can't predict what will update in it.
  • Imports are still gonna be broken (and with them, tests) still need to address that discussion.

@Kornil Kornil requested a review from blainekasten March 16, 2020 17:34
@pvdz
Copy link
Contributor

pvdz commented Apr 10, 2020

Hi. Fwiw, I'm working excessively in the run-queries.js file that's part of this commit. I didn't notice this PR before, sorry about that.

I have some more work left, part of the fast indexes effort, which I hope to wrap up in the next week or so. Meanwhile, expect some more churn like #22932 . Fwiw, I'm trying hard to be a good typing citizen, but the merge conflicts will be hard.

@blainekasten please hold off on merging this until I'm finished, or in between these PR's. (Although tbh I'm wrapping them up in fast cadence so that's hard). Thanks

import { getStore, onLogAction } from "../../redux/index"
import ReporterStore from "../../redux/index"

const { getStore, onLogAction } = ReporterStore
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for doing it in 2 steps? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason, @blainekasten fixed it by removing the object structure in the commits below, I highly prefer his solution.

@blainekasten blainekasten force-pushed the typescript-gatsby-cli-redux-folder branch from 842b8ea to 384eb5f Compare April 14, 2020 03:49
@pvdz
Copy link
Contributor

pvdz commented Apr 14, 2020

Fwiw, #22932 was merged and will cause merge conflicts.

I have more changes planned for this file, but not made them yet. If somebody can give me an estimate on how far away merging this PR is then I can potentially hold off on further changes. Otherwise I'll have to continue since this is blocking my work on fast filters.

@Kornil
Copy link
Contributor Author

Kornil commented Apr 14, 2020

@pvdz I can get the merge conflicts solved either later today or tomorrow. Not sure about plans for merging.

@blainekasten blainekasten force-pushed the typescript-gatsby-cli-redux-folder branch from 8b6a162 to 2c33e2a Compare April 15, 2020 06:01
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 835f2cf into gatsbyjs:master Apr 15, 2020
@gatsbot
Copy link

gatsbot bot commented Apr 15, 2020

Holy buckets, @Kornil — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

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.

4 participants