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): Translate redux/nodes to TypeScript #21010

Merged
merged 15 commits into from Mar 23, 2020
Merged

chore(gatsby): Translate redux/nodes to TypeScript #21010

merged 15 commits into from Mar 23, 2020

Conversation

@blainekasten
Copy link
Contributor

blainekasten commented Jan 29, 2020

Description

This PR does a few things.

  1. Greatly expands the coverage of types for redux state
  2. Adds types to redux/nodes.ts
  3. Changes a function to return an array instead of modify an array argument (I'll add inline comments to show)
  4. Change some sort code to utilize a single common type. (I'll add inline comments to show)
@blainekasten blainekasten requested a review from gatsbyjs/core as a code owner Jan 29, 2020
@blainekasten blainekasten changed the title [WIP] TS - Redux Nodes TS - Redux Nodes Jan 30, 2020

exports.addResolvedNodes = addResolvedNodes
return resolvedNodes
}

This comment has been minimized.

Copy link
@blainekasten

blainekasten Jan 30, 2020

Author Contributor

This function was changed to create an array and return it instead of modifying the argument array. Having an idempotent function feels better than having one with side effects. This function was only used in one place, and its usage was identical to this:

https://github.com/gatsbyjs/gatsby/pull/21010/files#diff-5d2176cb542d5c01d884139970be33fbR314

This comment has been minimized.

Copy link
@pvdz

pvdz Jan 31, 2020

Contributor

Need to check this. Could be faster, actually. Could also be slower. (Consider the case where each type has 100k nodes and they get merged. Before they were pushed onto the same node array, after they are pushed in their own array and later merged through a _.flatten() (which should really just be a .concat() imo)).

This comment has been minimized.

Copy link
@pvdz

pvdz Mar 10, 2020

Contributor

I checked this. It's slower, for sure, but in the case of the 150k ifsc benchmark it adds 5 to 10 seconds. I don't like slowing the build down for the user for the sake of something that only affects us at dev time, but I think the right choice is to do it anyways in this case ... :/

@pvdz pvdz changed the title TS - Redux Nodes typing(gatsby): TS - Redux Nodes Jan 31, 2020
Copy link
Contributor

pvdz left a comment

(I don't know if typing() is a proper lerna thing. Probably just chore(), but we should look into that)

(To the point of Greatly expands the coverage of types for redux state; I think it would be good if we had a coverage report included in PRs.)

This touches perf sensitive stuff and we need to benchmark it first. Numbers will lead the way.

packages/gatsby/src/redux/nodes.js Show resolved Hide resolved
packages/gatsby/src/redux/nodes.ts Outdated Show resolved Hide resolved
packages/gatsby/src/redux/nodes.ts Outdated Show resolved Hide resolved

exports.addResolvedNodes = addResolvedNodes
return resolvedNodes
}

This comment has been minimized.

Copy link
@pvdz

pvdz Jan 31, 2020

Contributor

Need to check this. Could be faster, actually. Could also be slower. (Consider the case where each type has 100k nodes and they get merged. Before they were pushed onto the same node array, after they are pushed in their own array and later merged through a _.flatten() (which should really just be a .concat() imo)).

packages/gatsby/src/redux/nodes.ts Outdated Show resolved Hide resolved
packages/gatsby/src/redux/nodes.ts Outdated Show resolved Hide resolved
packages/gatsby/src/redux/nodes.ts Outdated Show resolved Hide resolved
packages/gatsby/src/redux/run-sift.js Show resolved Hide resolved
Copy link
Contributor

pvdz left a comment

Did a fairly thorough review. It looks good! I do have a few questions.

.eslintrc.js Show resolved Hide resolved
},
})
for (const node of nodes.values()) {
const resolved = await resolver(node)

This comment has been minimized.

Copy link
@pvdz

pvdz Mar 11, 2020

Contributor

Isn't it possible that the first returned promise is actually this one? So the return value wouldn't be undefined, but whatever resolved is.

I'm a little surprised TS is accepting that. Maybe because Resolver is typed as any.

This comment has been minimized.

Copy link
@blainekasten

blainekasten Mar 13, 2020

Author Contributor

Do you have any idea what it should be typed as? I don't yet know what this resolver really is.

This comment has been minimized.

Copy link
@pvdz

pvdz Mar 13, 2020

Contributor

No, not when I was reviewing it. So if TS is fine with it, I guess let's just keep it like this and revisit that later.

packages/gatsby/src/redux/nodes.ts Outdated Show resolved Hide resolved
packages/gatsby/src/redux/nodes.ts Show resolved Hide resolved
packages/gatsby/src/redux/types.ts Outdated Show resolved Hide resolved
packages/gatsby/src/redux/types.ts Outdated Show resolved Hide resolved
packages/gatsby/src/redux/actions/internal.ts Outdated Show resolved Hide resolved
@blainekasten blainekasten changed the title typing(gatsby): TS - Redux Nodes chore(gatsby): Translate redux/nodes to TypeScript Mar 13, 2020
@blainekasten blainekasten requested a review from pvdz Mar 13, 2020
Copy link
Contributor

pvdz left a comment

Let's do this! 💃

@blainekasten blainekasten requested a review from pvdz Mar 23, 2020
@pvdz
pvdz approved these changes Mar 23, 2020
@gatsbybot gatsbybot merged commit ccf3ec2 into master Mar 23, 2020
20 of 23 checks passed
20 of 23 checks passed
build-test Workflow: build-test
Details
ci/circleci: starters_validate Your tests failed on CircleCI
Details
Gatsby Build Service Gatsby Build Service
Details
Cloud Tests
Details
Danger All good
Details
Peril All green. Well done.
Details
TypoCheck No typos found
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_structured_logging Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
ci/circleci: windows_unit_tests Your tests passed on CircleCI!
Details
cypress: default-group 139 tests passed in 02:57
Details
@delete-merged-branch delete-merged-branch bot deleted the ts/redux-nodes branch Mar 23, 2020
@pvdz

This comment has been minimized.

Copy link
Contributor

pvdz commented Mar 24, 2020

Published in gatsby@2.20.4 (just in case we need to bisect it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.