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

feat(gatsby): Allow awaiting API run triggered by createNode action #12748

Merged
merged 11 commits into from
May 14, 2019

Conversation

stefanprobst
Copy link
Contributor

When the createNode action is called, we currently have no way of knowing when subsequent onCreateNode API calls have finished. This is e.g. a problem in custom field resolvers, where we cannot safely call createRemoteFileNode.

This PR makes createNode a thunk which still synchronously dispatches the actions, but returns a Promise of the triggered API run that can optionally be awaited.

@stefanprobst stefanprobst requested a review from a team as a code owner March 22, 2019 05:35
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

In my opinion, this looks great!

I just have a question if you could elaborate on

When the createNode action is called, we currently have no way of knowing when subsequent onCreateNode API calls have finished. This is e.g. a problem in custom field resolvers, where we cannot safely call createRemoteFileNode.

why can't we safely call createRemoteFileNode?

packages/gatsby/src/redux/actions.js Outdated Show resolved Hide resolved
packages/gatsby/src/redux/actions.js Outdated Show resolved Hide resolved
@stefanprobst
Copy link
Contributor Author

why can't we safely call createRemoteFileNode?

Currently the createNode action is re-emitted as an event, which is caught in plugin-runner, which calls api-runner-node. The event-emitter sort of cuts off the feedback to the callsite.

The problem is that when we call createRemoteFileNode in a resolver, and want to e.g. query a childImageSharp field on that file node. When createRemoteFileNode returns we can be sure that the File node has been created (all is synchronous), but we need to await the subsequent onCreateNode API call as well that creates the ImageSharp child node. Otherwise, the childImageSharp field will be null.

@stefanprobst
Copy link
Contributor Author

Anything missing from this?

@DSchau DSchau added the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label Apr 1, 2019
@DSchau DSchau added this to In progress in OSS Roadmap via automation Apr 1, 2019
@DSchau
Copy link
Contributor

DSchau commented Apr 15, 2019

Thinking this looks good--let's get this merged today?

@DSchau
Copy link
Contributor

DSchau commented Apr 29, 2019

@pieh want to take a look at this soon?

@pieh
Copy link
Contributor

pieh commented Apr 29, 2019

@pieh want to take a look at this soon?

Yes, on it.

@pieh
Copy link
Contributor

pieh commented Apr 29, 2019

I just verified using this simple snippet:

---
title: Hello World
date: "2015-05-01T22:12:03.284Z"
remoteURL: "https://images.unsplash.com/photo-1556379092-dca659792591"
---
exports.sourceNodes = ({ actions, schema, store, cache, createNodeId }) => {
  actions.createTypes(`
  type MarkdownRemark implements Node {
    frontmatter: Frontmatter
  }
  `)
  actions.createTypes(
    schema.buildObjectType({
      name: "Frontmatter",
      fields: {
        remoteURL: {
          type: `File`,
          resolve: async (source, args, context, info) => {
            const fieldValue = source[info.fieldName]
            console.log({ fieldValue })

            const fileNode = await createRemoteFileNode({
              url: fieldValue,
              store,
              cache,
              createNode: actions.createNode,
              createNodeId,
              ext: `.jpg`,
            })

            return fileNode
          },
        },
      },
    })
  )
}

that this indeed works as expected. We will need to look at bumping/adjusting query running concurrency later on (because using above will limit downloading files to be just from 4 queries, and this can be bottleneck). Will run few more tests (against our .org and few other sites to verify there isn't any unintentional breakage somewhere, but code LGTM

@pieh
Copy link
Contributor

pieh commented Apr 29, 2019

Also additional note - (not a blocker) - right now, fact that createNode will return Promise is not documented. I'm not 100% sure where it should be documented - at least to action signature in API reference I think can be adjusted to note that Promise is returned, and when we have actually docs that cover using await SomethingThatUsesCreateNode in resolvers (so probably some advanced schema docs), then we could add cross linking

Co-Authored-By: stefanprobst <stefan.probst@univie.ac.at>
@stefanprobst
Copy link
Contributor Author

We will need to look at bumping/adjusting query running concurrency later on

yes good point!

@stefanprobst
Copy link
Contributor Author

fact that createNode will return Promise is not documented

Added short note about the returned Promise

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.

LGTM

@pieh pieh removed the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label May 14, 2019
@pieh pieh added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label May 14, 2019
@pieh pieh merged commit 17a67a5 into gatsbyjs:master May 14, 2019
OSS Roadmap automation moved this from In progress to Done May 14, 2019
@stefanprobst stefanprobst deleted the async-createnode-action branch May 17, 2019 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
No open projects
OSS Roadmap
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants