refactor(gatsby): change bootstrap to use services #24816

ascorbic · 2020-06-05T15:36:47Z

This splits the bootstrap command into smaller parts, which it imports from services. A lot of this is in the initialize service, which is mostly a direct TypeScript port from the relevant section of the old bootstrap file.

The services that this PR includes are:

initialize
customize-schema
source-nodes
build-schema
create-pages
create-pages-statefully
extract-queries
write-out-requires
write-out-redirects
post-bootstrap

packages/gatsby/src/bootstrap/index.ts

pieh · 2020-06-16T12:57:22Z

packages/gatsby/src/bootstrap/index.ts

+import { Runner } from "./create-graphql-runner"
+import { writeOutRedirects } from "../services/write-out-redirects"
+import { postBootstrap } from "../services/post-bootstrap"
+import { rebuildWithSitePage } from "../schema"


This one feels werid to actually not be service given that everything else is (other than types and one "startListener").

What qualifies something to be a service really?

In short: it being used in state machine. I'm not 100% sure whether I'll need to use this one there, but you're right I should probably make it one anyway.

I don't think this one will be used in any other place (schema-hot-reloader uses rebuild from src/schema/index), but I do like "entry points" for each ... "service" to be in same place (src/services/) vs some in services and some elsewhere.

Also another NIT - this change as is lost the update schema activity wrapping the execution of rebuildWithSitePage

activity = reporter.activityTimer(`update schema`, { parentSpan: bootstrapSpan, })

So if this is moved to service - that would be natural place to re-add it

wardpeet · 2020-06-18T07:57:45Z

I'll run this locally but looks good to me. Good job!

pvdz · 2020-06-18T12:30:26Z

packages/gatsby/src/commands/build.ts

@@ -70,8 +70,8 @@ module.exports = async function build(program: IBuildArgs): Promise<void> {
  const buildSpan = buildActivity.span
  buildSpan.setTag(`directory`, program.directory)

-  const { graphqlRunner: bootstrapGraphQLRunner } = await bootstrap({
-    ...program,
+  const { gatsbyNodeGraphQLFunction } = await bootstrap({


@ascorbic btw when I mean reducing irrelevant changes out, this is the kind. This file has 5 changed lines. Three of them concern a var rename. I bet we can find many of these that are not really relevant to the core of this PR.

You can probably separate out the bootstrap api changes as well. (Export default to export binding, program no longer being spread, returned keys, etc).

In other words; I don't think any of the changes in this file are really relevant to the kind of change this PR intends to introduce.

pvdz

I did a thorough review.

It seems to me like there's a lot of duplicate code going on in the service files. Every service is basically the pattern

const activity = reporter.activityTimer(desc, {
    parentSpan,
  })
  activity.start()
  await <action>
  activity.end()

Can't we abstract and dedupe that code? Do we prefer to be explicit in this case?

Beyond that I have some non-blocking comments, a question, and two minor changes I'd like to see (moving the type stuff and removing debug comments). This is almost ready to go :)

packages/gatsby/src/services/build-schema.ts

packages/gatsby/src/services/create-pages.ts

packages/gatsby/src/services/initialize.ts

pvdz · 2020-06-19T10:37:27Z

packages/gatsby/src/services/initialize.ts

-}
-
-module.exports = async (args: BootstrapArgs) => {
-  const spanArgs = args.parentSpan ? { childOf: args.parentSpan } : {}


I presume this childOf stuff is not relevant anymore?

packages/gatsby/src/services/initialize.ts

packages/gatsby/src/services/post-bootstrap.ts

Co-authored-by: Peter van der Zee <209817+pvdz@users.noreply.github.com>

ascorbic · 2020-06-19T12:03:25Z

@pvdz Thanks for the detailed review! Regarding that activity stuff, yes it is annoying to have all that boilerplate, though I'm not sure how much shorter it would be if I abstracted it: I'd only really be able to remove the explicit start() call, as everything else would still need passing in, and it would still need stopping. Any ideas for a cleaner way?

wardpeet · 2020-06-19T12:05:00Z

@pdvz Let's not refactor activities. I like the verbosity of it + we might need to think about another way in the future for this. In state machine land these could be side effects

… into chore/bootstrap-refactor

pvdz

🚀

muescha · 2020-06-21T20:17:50Z

packages/gatsby/src/commands/types.ts

@@ -15,14 +16,15 @@ export interface IProgram {
  port: number
  proxyPort: number
  host: string
-  report: typeof reporter
+  report: Reporter


somehow this reverts this PR:

refactor(gatsby): convert program reducer to typescript #24941 refactor(gatsby): convert program reducer to typescript

- report: Reporter + report: typeof reporter

was this intended?

//cc @tgallacher

ascorbic added 8 commits June 2, 2020 16:25

Add typings for action arrays and thunks

3745526

Switch back to non-null assertion

e2e40ed

Add comment

0f3a2c0

Refactor bootstrap into services

d4596f8

Use separate gql runner for bootstrap

37e005b

Import reporter typings

a6af772

Switch to new bootstrap

381415a

Fix imports

603a501

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 5, 2020

ascorbic added topic: develop state machine and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 5, 2020

ascorbic requested review from wardpeet, freiksenet, pieh and sidharthachatterjee June 5, 2020 15:39

blainekasten mentioned this pull request Jun 5, 2020

[Umbrella] TypeScript Migration (Help wanted!) #21995

Closed

ascorbic added 6 commits June 5, 2020 17:19

Merge branch 'chore/ts/action-types' into chore/bootstrap-refactor

5f31fcb

Add create pages

564140d

Add extract queries

6ce9605

Fix typings

948aa0a

Rebuild schema with sitepage

8fdfec3

Rename context vars

adb1829

ascorbic marked this pull request as ready for review June 11, 2020 10:49

ascorbic requested a review from a team as a code owner June 11, 2020 10:49

This was referenced Jun 12, 2020

refactor(gatsby): Change develop command to use startWebpackServer service #24946

Merged

WIP: Develop state machine tracker PR #24948

Closed

ascorbic added the status: needs core review Currently awaiting review from Core team member label Jun 12, 2020

Fix types

e138095

pieh reviewed Jun 16, 2020

View reviewed changes

packages/gatsby/src/bootstrap/index.ts Outdated Show resolved Hide resolved

pieh reviewed Jun 16, 2020

View reviewed changes

ascorbic added 2 commits June 16, 2020 16:08

Changes from review

318421b

Merge branch 'master' into chore/bootstrap-refactor

a7f93bd

ascorbic changed the title ~~refactor(gatsby): change boostrap to use services~~ refactor(gatsby): change bootstrap to use services Jun 16, 2020

ascorbic added 3 commits June 16, 2020 17:21

Bikeshed the function name

a418282

Better unhandled rejection handling

e0c4380

Merge branch 'master' into chore/bootstrap-refactor

ce35d9f

sidharthachatterjee self-assigned this Jun 17, 2020

Merge branch 'master' into chore/bootstrap-refactor

15527f5

pvdz reviewed Jun 18, 2020

View reviewed changes

pvdz suggested changes Jun 19, 2020

View reviewed changes

ascorbic and others added 2 commits June 19, 2020 12:32

Merge branch 'master' into chore/bootstrap-refactor

2240109

Update packages/gatsby/src/services/initialize.ts

f087d53

Co-authored-by: Peter van der Zee <209817+pvdz@users.noreply.github.com>

wardpeet previously approved these changes Jun 19, 2020

View reviewed changes

ascorbic added 2 commits June 19, 2020 13:08

Changes from review

dd3d1c5

Merge branch 'chore/bootstrap-refactor' of github.com:gatsbyjs/gatsby…

6c0f22d

… into chore/bootstrap-refactor

ascorbic dismissed wardpeet’s stale review via 6c0f22d June 19, 2020 12:08

ascorbic requested review from wardpeet and pvdz June 19, 2020 12:11

pvdz previously approved these changes Jun 19, 2020

View reviewed changes

Format

94d22ae

ascorbic dismissed pvdz’s stale review via 94d22ae June 19, 2020 12:13

pvdz approved these changes Jun 19, 2020

View reviewed changes

ascorbic added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jun 19, 2020

gatsbybot merged commit a78c7b0 into master Jun 19, 2020

ascorbic deleted the chore/bootstrap-refactor branch June 19, 2020 13:17

muescha reviewed Jun 21, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(gatsby): change bootstrap to use services #24816

refactor(gatsby): change bootstrap to use services #24816

ascorbic commented Jun 5, 2020 •

edited

Loading

pieh Jun 16, 2020

ascorbic Jun 16, 2020

pieh Jun 16, 2020

wardpeet commented Jun 18, 2020

pvdz Jun 18, 2020 •

edited

Loading

pvdz left a comment

pvdz Jun 19, 2020

ascorbic commented Jun 19, 2020

wardpeet commented Jun 19, 2020

pvdz left a comment

muescha Jun 21, 2020

refactor(gatsby): change bootstrap to use services #24816

refactor(gatsby): change bootstrap to use services #24816

Conversation

ascorbic commented Jun 5, 2020 • edited Loading

pieh Jun 16, 2020

Choose a reason for hiding this comment

ascorbic Jun 16, 2020

Choose a reason for hiding this comment

pieh Jun 16, 2020

Choose a reason for hiding this comment

wardpeet commented Jun 18, 2020

pvdz Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

pvdz left a comment

Choose a reason for hiding this comment

pvdz Jun 19, 2020

Choose a reason for hiding this comment

ascorbic commented Jun 19, 2020

wardpeet commented Jun 19, 2020

pvdz left a comment

Choose a reason for hiding this comment

muescha Jun 21, 2020

Choose a reason for hiding this comment

ascorbic commented Jun 5, 2020 •

edited

Loading

pvdz Jun 18, 2020 •

edited

Loading