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(cli): use fetch instead of yarn npm info #9975

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Feb 8, 2024

Some time ago I think I made the call to use yarn npm info to get information from NPM about a package's version because I didn't want to install yet another dependency. This was the wrong move in hindsight since it meant 1) spawning a child process and 2) while it seemed safe to assume that Redwood projects would have a version of yarn with the npm subcommand, this wasn't true in practice.

Now that fetch is in Node.js, we can just use that like @Josh-Walker-GM has elsewhere:

const packumentP = await fetch(`https://registry.npmjs.org/${module}`)
const packument = await packumentP.json()
// If the version includes a plus, like '4.0.0-rc.428+dd79f1726'
// (all @canary, @next, and @rc packages do), get rid of everything after the plus.
if (version.includes('+')) {
version = version.split('+')[0]
}
const versionIsPublished = Object.keys(packument.versions).includes(version)

I think this might fix the bug you saw when deploying yesterday.

@jtoar jtoar added the release:fix This PR is a fix label Feb 8, 2024
@jtoar jtoar added this to the v7.0.0 milestone Feb 8, 2024
packages/cli/src/commands/setup/auth/auth.js Outdated Show resolved Hide resolved
packages/cli/src/lib/packages.js Outdated Show resolved Hide resolved
redwood-bot and others added 2 commits February 8, 2024 08:34
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM left a comment

Choose a reason for hiding this comment

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

Agreed that doing an API request is likely an improvement over spawning a child process.

@jtoar jtoar enabled auto-merge (squash) February 8, 2024 18:37
@jtoar jtoar merged commit 180615b into main Feb 8, 2024
40 checks passed
@jtoar jtoar deleted the ds-cli/refactor-yarn-npm-info branch February 8, 2024 18:50
jtoar added a commit that referenced this pull request Feb 9, 2024
Some time ago I think I made the call to use `yarn npm info` to get
information from NPM about a package's version because I didn't want to
install yet another dependency. This was the wrong move in hindsight
since it meant 1) spawning a child process and 2) while it seemed safe
to assume that Redwood projects would have a version of yarn with the
`npm` subcommand, this wasn't true in practice.

Now that fetch is in Node.js, we can just use that like @Josh-Walker-GM
has elsewhere:


https://github.com/redwoodjs/redwood/blob/019361550736f5ed733c37d7f5241583f26d405b/packages/cli/src/commands/experimental/setupDockerHandler.js#L330-L339

I think this might fix the bug you saw when deploying yesterday.

---------

Co-authored-by: Dominic Saadi <dominiceliassaadi@gmail.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
dac09 added a commit to dac09/redwood that referenced this pull request Feb 9, 2024
…d-cookies-dbauth

* 'main' of github.com:redwoodjs/redwood:
  chore: update rsc fixture (redwoodjs#9986)
  fix(server): use file extension in import, fix graphql route registering (redwoodjs#9984)
  chore(deps): update babel monorepo (redwoodjs#9983)
  fix: unpin react types (redwoodjs#9727)
  fix(docker): compose dev and prod (redwoodjs#9982)
  fix(deps): update prisma monorepo to v5.9.1 (redwoodjs#9980)
  fix(cli): use fetch instead of `yarn npm info` (redwoodjs#9975)
  fix(test): Update createServer test to use a different port to normal (redwoodjs#9977)
  fix(docker): corepack permissions fix and style updates (redwoodjs#9976)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants