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

docs(gatsby-source-wordpress): Remove lodash + bluebird + fix Promise hell #11739

Merged
merged 6 commits into from
Feb 15, 2019
Merged

docs(gatsby-source-wordpress): Remove lodash + bluebird + fix Promise hell #11739

merged 6 commits into from
Feb 15, 2019

Conversation

DylanTackoor
Copy link
Contributor

Description

This commit:

  • Removes lodash in favor of Array.forEach
  • Bluebird in favor of the native Promise object
  • Avoids nested thenables by using Promise.all()

I've been using this code in my builds and it's been working fine for whatever that worth :)

This commit:
- Removes lodash in favor of Array.forEach
- Bluebird in favor of the native Promise object
- Avoids nested thenables by using Promise.all()

I've been using this code in my builds and it's been working fine for whatever that worth :)
@DylanTackoor
Copy link
Contributor Author

Looks like the Azure Pipeline task has a bug pulling something with Yarn, but the code works otherwise 😱

2019-02-13T15:46:04.3801464Z ##[section]Starting: CmdLine
2019-02-13T15:46:04.3911190Z ==============================================================================
2019-02-13T15:46:04.3911250Z Task         : Command Line
2019-02-13T15:46:04.3911288Z Description  : Run a command line script using cmd.exe on Windows and bash on macOS and Linux.
2019-02-13T15:46:04.3911345Z Version      : 2.146.1
2019-02-13T15:46:04.3911378Z Author       : Microsoft Corporation
2019-02-13T15:46:04.3911415Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkID=613735)
2019-02-13T15:46:04.3911468Z ==============================================================================
2019-02-13T15:46:05.7867622Z Generating script.
2019-02-13T15:46:05.7991716Z Script contents:
2019-02-13T15:46:05.7998822Z yarn bootstrap
2019-02-13T15:46:05.8597677Z ##[command]"C:\windows\system32\cmd.exe" /D /E:ON /V:OFF /S /C "CALL "D:\a\_temp\bd085075-59cf-4de0-85c0-3b4f065420e8.cmd""
2019-02-13T15:46:15.0653626Z yarn run v1.13.0
2019-02-13T15:46:15.1773258Z $ yarn
2019-02-13T15:46:16.5975153Z [1/4] Resolving packages...
2019-02-13T15:46:18.2236144Z [2/4] Fetching packages...
2019-02-13T15:47:28.6084712Z error An unexpected error occurred: "https://registry.yarnpkg.com/@xtuc/long/-/long-4.2.1.tgz: Request failed \"502 Bad Gateway\"".
2019-02-13T15:47:28.6085038Z info If you think this is a bug, please open a bug report with the information provided in "D:\\a\\1\\s\\yarn-error.log".
2019-02-13T15:47:28.6085885Z info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
2019-02-13T15:47:38.7696222Z error Command failed with exit code 1.
2019-02-13T15:47:38.7697806Z info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
2019-02-13T15:47:39.0497956Z ##[error]Cmd.exe exited with code '1'.
2019-02-13T15:47:39.0894802Z ##[section]Finishing: CmdLine

@DylanTackoor DylanTackoor changed the title Remove lodash and bluebird + fix Promise hell (gatsby-source-wordpress) Remove lodash and bluebird + fix Promise hell Feb 13, 2019
@DylanTackoor DylanTackoor changed the title (gatsby-source-wordpress) Remove lodash and bluebird + fix Promise hell (gatsby-source-wordpress) Remove lodash + bluebird + fix Promise hell Feb 13, 2019
@pieh pieh changed the title (gatsby-source-wordpress) Remove lodash + bluebird + fix Promise hell docs(gatsby-source-wordpress): Remove lodash + bluebird + fix Promise hell Feb 13, 2019
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Yeah - this is generally cleaner.

Would you be interested in refactoring with async/await? (see #11548 for more info on that!)

@DylanTackoor
Copy link
Contributor Author

Would you be interested in refactoring with async/await? (see #11548 for more info on that!)

I initially had an async/await version but decided on this approach as Promise.all() should be faster as both Promises execute in parallel this way :D

@DSchau
Copy link
Contributor

DSchau commented Feb 14, 2019

@DylanTackoor you can (and should!) use Promise.all with async/await ;)

Using one does not preclude the other.

@DylanTackoor
Copy link
Contributor Author

Sweet, thanks for the tip! I'll update the PR later today when I have a chance 😊

Feeling like this is a bit cleaner than the Promise.all() approach :)
@DylanTackoor
Copy link
Contributor Author

@DSchau I ended up avoiding using Promise.all() in favor of a single async GraphQL query. Thoughts?

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Looking better!

Unfortunately, we don't throw errors if a GraphQL query has errors (so you can still use the GraphiQL editor to try and debug queries), we use an errors property on the returned object from the graphql call.

So you'll want to remove the try/catch, and rather do something like

const result = await graphql(``)

if (result.errors) {
  throw new Error(result.errors)
}

if you want to stop execution and alert of errors.

Moved from try/catch to checking if results.errors exits
@DylanTackoor
Copy link
Contributor Author

Fixed it! Also took the chance to destructure the data property.

@wardpeet
Copy link
Contributor

Merged master and ran prettier

@DSchau mind taking another look?

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Let's do it; thanks for this, think it's a big improvement! Also co-locating the queries instead of a Promise.all was a nice tweak!

@DSchau DSchau merged commit f7a5a35 into gatsbyjs:master Feb 15, 2019
@gatsbot
Copy link

gatsbot bot commented Feb 15, 2019

Holy buckets, @DylanTackoor — 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. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  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!

DSchau pushed a commit that referenced this pull request Feb 28, 2019
<!--
  Have any questions? Check out the contributing docs at https://gatsby.app/contribute, or
  ask in this Pull Request and a Gatsby maintainer will be happy to help :)
-->

## Description

Updates example site gatsby-node.js to match documentation.

## Related Issues

Previously implemented in #11739.
@DylanTackoor DylanTackoor deleted the patch-1 branch March 15, 2019 02:08
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.

None yet

3 participants