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

fix(gatsby-recipes): fix NPMScript for parallel calls #26349

Merged
merged 4 commits into from
Aug 11, 2020

Conversation

KyleAMathews
Copy link
Contributor

Found this was problem similar to NPMPackageJSON in #26281

Did fancy batching technique there like with NPMPackage but realized that was overkill if writing is cheap.

Install npm package is very expensive so we want to batch those. Writing to package.json is extreamly cheap
so using a simple locking mechanism is a lot easier so added that to NPMScript & replaced the batching
code in NPMPackageJSON to use the same locking (which was necessary anyways to make sure NPMScript & NPMPackageJSON
don't clobber each other).

@KyleAMathews KyleAMathews requested review from a team as code owners August 11, 2020 01:14
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Aug 11, 2020
@gatsby-cloud-staging
Copy link

gatsby-cloud-staging bot commented Aug 11, 2020

Gatsby Cloud Build Report

🚩 Your build failed. See the build logs here

Errors

Error in "/usr/src/app/www/examples/gatsbygram/node_modules/gatsby-plugin-page-creator/gatsby-node.js": Cannot find module 'gatsby-cli/lib/reporter'
Require stack:
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby-plugin-page-creator/is-valid-collection-path-implementation.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby-plugin-page-creator/create-pages-from-collection-builder.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby-plugin-page-creator/create-page-wrapper.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby-plugin-page-creator/gatsby-node.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bootstrap/resolve-module-exports.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bootstrap/load-plugins/validate.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bootstrap/load-plugins/load.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bootstrap/load-plugins/index.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/services/initialize.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/services/index.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bootstrap/index.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/commands/build.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/node_modules/gatsby-cli/lib/create-cli.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/node_modules/gatsby-cli/lib/index.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bin/gatsby.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/cli.js

@gatsby-cloud
Copy link

gatsby-cloud bot commented Aug 11, 2020

Gatsby Cloud Build Report

🚩 Your build failed. See the build logs here

Errors

Error in "/usr/src/app/www/www/node_modules/gatsby-plugin-page-creator/gatsby-node.js": Cannot find module 'gatsby-cli/lib/reporter'
Require stack:
- /usr/src/app/www/www/node_modules/gatsby-plugin-page-creator/is-valid-collection-path-implementation.js
- /usr/src/app/www/www/node_modules/gatsby-plugin-page-creator/create-pages-from-collection-builder.js
- /usr/src/app/www/www/node_modules/gatsby-plugin-page-creator/create-page-wrapper.js
- /usr/src/app/www/www/node_modules/gatsby-plugin-page-creator/gatsby-node.js
- /usr/src/app/www/www/node_modules/gatsby/dist/bootstrap/resolve-module-exports.js
- /usr/src/app/www/www/node_modules/gatsby/dist/bootstrap/load-plugins/validate.js
- /usr/src/app/www/www/node_modules/gatsby/dist/bootstrap/load-plugins/load.js
- /usr/src/app/www/www/node_modules/gatsby/dist/bootstrap/load-plugins/index.js
- /usr/src/app/www/www/node_modules/gatsby/dist/services/initialize.js
- /usr/src/app/www/www/node_modules/gatsby/dist/services/index.js
- /usr/src/app/www/www/node_modules/gatsby/dist/bootstrap/index.js
- /usr/src/app/www/www/node_modules/gatsby/dist/commands/build.js
- /usr/src/app/www/www/node_modules/gatsby/node_modules/gatsby-cli/lib/create-cli.js
- /usr/src/app/www/www/node_modules/gatsby/node_modules/gatsby-cli/lib/index.js
- /usr/src/app/www/www/node_modules/gatsby/dist/bin/gatsby.js
- /usr/src/app/www/www/node_modules/gatsby/cli.js

@KyleAMathews KyleAMathews added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: recipes and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Aug 11, 2020
}
})
} else {
right.properties.unshift(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mxstbr was fixing this parallelism support & also added blank slate support i.e. when gatsby-config.js doesn't have siteMetadata added.

Copy link
Contributor

Choose a reason for hiding this comment

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

💯 This is great, thank you!!

Copy link
Contributor

@johno johno left a comment

Choose a reason for hiding this comment

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

🔐

@KyleAMathews KyleAMathews merged commit e845615 into master Aug 11, 2020
@LekoArts LekoArts deleted the parallel-fixes branch November 9, 2020 08:27
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
* Add tests for parallel NPMPackageJSON

* Add way to acquire/release locks on shared resources to safely read/write from them

* Also lock writes to gatsby-config.js

* Lock gatsby-config.js for site-metadata.js as well + make work when config doesn't have siteMetadata field
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants