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): migrate pages reducer to TypeScript #23701

Merged
merged 1 commit into from
May 5, 2020
Merged

chore(gatsby): migrate pages reducer to TypeScript #23701

merged 1 commit into from
May 5, 2020

Conversation

hiwelo
Copy link
Contributor

@hiwelo hiwelo commented May 2, 2020

Description

This PR migrates the gatsby/src/reducers/pages file to TypeScript.
It requires the addition of a new package in the devDependencies of the project, @types/normalize-path from the DefinitelyTyped project.

To achieve the migration, I had to update the IGatsbyPlugin interface with an optional id property, and to fix what appeared to be a typo for the pluginCreator___NODE property of the CREATE_PAGE action payload.

Related issues

Relates to #21995

@hiwelo hiwelo self-assigned this May 2, 2020
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 2, 2020
@blainekasten blainekasten mentioned this pull request May 2, 2020
Closed
@hiwelo hiwelo added type: TypeScript migration and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 2, 2020
@hiwelo hiwelo requested review from blainekasten and a team May 2, 2020 13:32
@hiwelo hiwelo force-pushed the ts-migration/reducer-pages branch 3 times, most recently from 3fa83aa to 8d55794 Compare May 2, 2020 21:11
@hiwelo hiwelo marked this pull request as ready for review May 2, 2020 21:59
ascorbic
ascorbic suggested changes May 4, 2020
View reviewed changes
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Thanks! Just a couple of minor suggestions.

packages/gatsby/src/redux/reducers/pages.ts Outdated Show resolved Hide resolved
packages/gatsby/src/redux/types.ts Outdated Show resolved Hide resolved
packages/gatsby/src/redux/types.ts Show resolved Hide resolved
@ascorbic ascorbic self-assigned this May 4, 2020
@hiwelo hiwelo force-pushed the ts-migration/reducer-pages branch from 8d55794 to 650ce3a Compare May 4, 2020 10:43
@hiwelo
Copy link
Contributor Author

hiwelo commented May 4, 2020 •
edited
Loading

@ascorbic Thanks for the review. I amended my commit with your suggestions. 👍

People gathering their hands together, with a dog adding its paw on the top, from the movie Airbud

ascorbic
ascorbic approved these changes May 4, 2020
View reviewed changes
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@ascorbic ascorbic added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label May 4, 2020
ascorbic
ascorbic suggested changes May 4, 2020
View reviewed changes
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

OK, looks like the change to checking just id means it's failing tests. Can you change it and make sure it passes? Thanks.

@ascorbic ascorbic removed the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label May 4, 2020
@hiwelo hiwelo force-pushed the ts-migration/reducer-pages branch from 9ef68fa to 80a3fea Compare May 4, 2020 16:57
@hiwelo
Copy link
Contributor Author

hiwelo commented May 4, 2020 •
edited
Loading

It looks like it's more than just one change. Tests are relying on a series of pages created without id which is triggering a series of failing tests.

Because of that, I rolled back to the previous situation (with successful tests) based on checking name. I would check all pages-related tests in another PR to make it clearer imo; as this PR focuses on migrating the file. I can have a look once merged and open a new PR for that.
Or do you prefer doing everything in the same PR?

ascorbic
ascorbic suggested changes May 5, 2020
View reviewed changes
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

I agree that there are too many places that the tests could break, so the approach you've taken makes sense. I'm ready to approve save one nit.

packages/gatsby/src/redux/reducers/pages.ts Outdated Show resolved Hide resolved
@hiwelo hiwelo force-pushed the ts-migration/reducer-pages branch from 80a3fea to 75a380a Compare May 5, 2020 13:31
@hiwelo hiwelo force-pushed the ts-migration/reducer-pages branch from 75a380a to 5d2bb27 Compare May 5, 2020 13:34
ascorbic
ascorbic approved these changes May 5, 2020
View reviewed changes
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Looks great!

@ascorbic ascorbic added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label May 5, 2020
@gatsbybot gatsbybot merged commit eaa2f3f into master May 5, 2020
@delete-merged-branch delete-merged-branch bot deleted the ts-migration/reducer-pages branch May 5, 2020 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ascorbic ascorbic ascorbic approved these changes

@blainekasten blainekasten Awaiting requested review from blainekasten

Assignees

@ascorbic ascorbic

@hiwelo hiwelo

Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hiwelo @ascorbic @gatsbybot