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

Missing yarn.lock from the init repo breaks Yarn installs #28238

Closed
arcanis opened this issue Nov 23, 2020 · 6 comments · Fixed by #28260
Closed

Missing yarn.lock from the init repo breaks Yarn installs #28238

arcanis opened this issue Nov 23, 2020 · 6 comments · Fixed by #28260
Labels
topic: cli Related to the Gatsby CLI type: bug An issue or pull request relating to a bug in Gatsby

Comments

@arcanis
Copy link
Contributor

arcanis commented Nov 23, 2020

Description

The following commit removed the yarn.lock from gatsby-starter-default:

gatsbyjs/gatsby-starter-default@a5368fa#diff-51e4f558fae534656963876761c95b83b6ef5da5103c4adef6768219ed76c2de

As a result, the init script now goes through the following codepath:

if (getPackageManager() === `yarn` && checkForYarn()) {
if (await fs.pathExists(`package-lock.json`)) {
if (!(await fs.pathExists(`yarn.lock`))) {
await spawn(`yarnpkg import`)
}
await fs.remove(`package-lock.json`)
}

The yarn import command has been deprecated / removed in Yarn 2, and crashes.

Steps to reproduce

cd $(mktemp -d)
yarn init -2 -w
yarn dlx gatsby new my-gatsby

Expected result

Works

Actual result

Breaks

@arcanis arcanis added the type: bug An issue or pull request relating to a bug in Gatsby label Nov 23, 2020
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 23, 2020
@arcanis
Copy link
Contributor Author

arcanis commented Nov 23, 2020

Seems to have broke starting from #26856, cc @laurieontech

@pieh pieh added topic: cli Related to the Gatsby CLI and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 23, 2020
@gillkyle
Copy link
Contributor

Thanks for reporting this bug!

I was able to reproduce and will provide some ideas, though probably need @laurieontech's feedback on the why of the lock files removal. My understanding is that the deprecation of yarn import is what is causing this to break, and the missing yarn.lock files is just what is causing this code to get to those declarations, does that seem like a fair assessment @arcanis?

Here's my output running: npx yarn@berry dlx gatsby new my-gatsby-3

Erroneous output
info Creating new site from git: https://github.com/gatsbyjs/gatsby-starter-default.git
Cloning into 'my-gatsby-3'...
remote: Enumerating objects: 28, done.
remote: Counting objects: 100% (28/28), done.
remote: Compressing objects: 100% (26/26), done.
remote: Total 28 (delta 2), reused 13 (delta 0), pack-reused 0
Unpacking objects: 100% (28/28), done.
success Created starter directory layout
info Installing packages...
Usage Error: The nearest package directory (/Users/kyle/tmp/issue28238/my-gatsby-3) doesn't seem to be part of the project declared in /Users/kyle/tmp/issue28238.

- If the project directory is right, it might be that you forgot to list my-gatsby-3 as a workspace.
- If it isn't, it's likely because you have a yarn.lock or package.json file there, confusing the project root detection.

$ yarn run [--inspect] [--inspect-brk]  ...
error Command failed with exit code 1: yarnpkg import


  Error: Command failed with exit code 1: yarnpkg import

  - error.js:56 makeError
    [lib]/[gatsby-cli]/[execa]/lib/error.js:56:11

  - index.js:114 handlePromise
    [lib]/[gatsby-cli]/[execa]/index.js:114:26

  - task_queues.js:97 processTicksAndRejections
    internal/process/task_queues.js:97:5

  - init-starter.js:133 install
    [lib]/[gatsby-cli]/lib/init-starter.js:133:11

  - init-starter.js:209 clone
    [lib]/[gatsby-cli]/lib/init-starter.js:209:3

  - init-starter.js:350 initStarter
    [lib]/[gatsby-cli]/lib/init-starter.js:350:5

  - create-cli.js:443
    [lib]/[gatsby-cli]/lib/create-cli.js:443:7

The key part being: error Command failed with exit code 1: yarnpkg import

Since Yarn Modern removes that command, I'm wondering if we can just skip that invocation entirely for Yarn Modern (or maybe just entirely) and generate the lock file from when yarn is called below it?

@arcanis
Copy link
Contributor Author

arcanis commented Nov 24, 2020

My understanding is that the deprecation of yarn import is what is causing this to break, and the missing yarn.lock files is just what is causing this code to get to those declarations, does that seem like a fair assessment @arcanis?

Not quite; due to both architectural differences between our package managers and the complexity of keeping the command forward-compatible with future npm versions, running yarn import as part of a classic project bootstrap is not a recommended practice regardless of the context.

My understanding is that the code behaved this way to have low-cost best-effort compatibility with templates that didn't use Yarn; running this option should only be seen as a last resort option, and frankly I feel it'd be better to have no import than try to auto-import an npm one.

Since Yarn Modern removes that command, I'm wondering if we can just skip that invocation entirely for Yarn Modern (or maybe just entirely) and generate the lock file from when yarn is called below it?

That's fine by me, although it's too bad the perfs will be made worse on Yarn because of the drop of the lockfile. What's the reason for removing it in the first place?

@mxstbr
Copy link
Contributor

mxstbr commented Nov 24, 2020

What's the reason for removing it in the first place?

We want users to get the latest versions of the dependencies that match the semver version from package.json, not the versions specified in the lockfile!

@merceyz
Copy link
Contributor

merceyz commented Nov 24, 2020

If that's the case shouldn't package-lock.json have been deleted as well?

@ascorbic
Copy link
Contributor

My understanding was that we were only removing it from the minimal starter because it was targetting a canary, and with that we were removing both lockfiles. I don't know why we'd remove it from others.

ascorbic added a commit that referenced this issue Nov 24, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
#28238 (comment)
gatsbybot added a commit to gatsbyjs/gatsby-starter-blog that referenced this issue Nov 24, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
gatsbybot added a commit to gatsbyjs/gatsby-starter-default that referenced this issue Nov 24, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
gatsbybot added a commit to gatsbyjs/gatsby-starter-blog-theme that referenced this issue Nov 24, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
gatsbybot added a commit to gatsbyjs/gatsby-starter-blog-theme-core that referenced this issue Nov 24, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
gatsbybot added a commit to gatsbyjs/gatsby-starter-mdx-basic that referenced this issue Nov 24, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
gatsbybot added a commit to gatsbyjs/gatsby-starter-notes-theme that referenced this issue Nov 24, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
gatsbybot added a commit to gatsbyjs/gatsby-starter-plugin that referenced this issue Nov 24, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
gatsbybot added a commit to gatsbyjs/gatsby-starter-theme that referenced this issue Nov 24, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
gatsbybot added a commit to gatsbyjs/gatsby-starter-hello-world that referenced this issue Nov 24, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
vladar pushed a commit that referenced this issue Nov 25, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
#28238 (comment)

(cherry picked from commit f6417dd)
vladar added a commit that referenced this issue Nov 25, 2020
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
#28238 (comment)

(cherry picked from commit f6417dd)

Co-authored-by: Matt Kane <[email protected]>
fumierkm added a commit to fumierkm/gatsbyjs that referenced this issue Nov 27, 2021
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
goldenjake pushed a commit to goldenjake/gatsby-starter-default that referenced this issue May 19, 2022
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
leonhiat added a commit to leonhiat/gatsby-starter-blog that referenced this issue Oct 31, 2023
* Fix starter publish script

* Don't use yarn import because @arcanis says it's a bad idea
gatsbyjs/gatsby#28238 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: cli Related to the Gatsby CLI type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants