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: support other package managers #895

Merged

Conversation

DaniAkash
Copy link
Contributor

🚥 Resolves #894

🧰 Changes

Added a detection logic for deno, bun, yarn & pnpm before running the install command.

🧬 QA & Testing

Running this project in the above environments to try if the package gets installed correctly.

erunion
erunion previously approved these changes Jun 10, 2024
@erunion erunion requested a review from kanadgupta June 10, 2024 17:18
@erunion erunion added enhancement New feature or request area:api Issues related to the `api` CLI, which builds the SDKs labels Jun 10, 2024
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

Per @erunion's comment, could you use preferred-pm instead? I'd feel much better about deferring to a maintained library with coverage for this logic as opposed to maintaining it ourselves. Thanks @DaniAkash!

@erunion erunion dismissed their stale review June 11, 2024 21:36

see comments

@DaniAkash
Copy link
Contributor Author

Per @erunion's comment, could you use preferred-pm instead? I'd feel much better about deferring to a maintained library with coverage for this logic as opposed to maintaining it ourselves. Thanks @DaniAkash!

Sure! I'll add the change 👍🏽

Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

Thanks @DaniAkash!

@kanadgupta kanadgupta merged commit 21230f8 into readmeio:main Jun 26, 2024
5 checks passed
@DaniAkash DaniAkash deleted the dani/support-other-package-managers branch July 3, 2024 07:22
@joscha
Copy link

joscha commented Aug 28, 2024

has this landed on main?

I am getting:

✖ Installing required packages
Command failed with exit code 1: npm install --save /Users/joscha/dev/.../.api/apis/affinity
npm error Cannot read properties of null (reading 'matches')

npm error A complete log of this run can be found in: /Users/joscha/.npm/_logs/2024-08-28T10_53_59_578Z-debug-0.log

when running api install with

  • deno 1.45.5
  • api 6.1.2

I can't find any mention of #894 in the changelog on https://github.com/readmeio/api/releases, however 21230f8 I can see it is part of v7.0.0-beta.8, though when running beta.8 via

deno run --allow-read --allow-run --allow-env --allow-sys --allow-write=.api npm:api@7.0.0-beta.8 install ./openapi/2024-08-28.json

I still get:

✖ Installing required packages
Command failed with exit code 1: npm install --save /Users/joscha/dev/.../.api/apis/affinity
npm warn ERESOLVE overriding peer dependency
npm warn ERESOLVE overriding peer dependency
npm warn While resolving: @nestjs/axios@3.0.2
npm warn Found: peer reflect-metadata@"^0.1.12 || ^0.2.0" from @nestjs/common@10.4.1
npm warn node_modules/@nestjs/common
npm warn   peer @nestjs/common@"^10.0.0" from @nestjs/platform-express@10.3.2
npm warn   node_modules/@nestjs/platform-express
npm warn   1 more (@nestjs/core)
npm warn
npm warn Could not resolve dependency:
npm warn peer reflect-metadata@"^0.1.12 || ^0.2.0" from @nestjs/common@10.4.1
npm warn node_modules/@nestjs/common
npm warn   peer @nestjs/common@"^10.0.0" from @nestjs/platform-express@10.3.2
npm warn   node_modules/@nestjs/platform-express
npm warn   1 more (@nestjs/core)
npm error Cannot read properties of null (reading 'matches')

it still seems to invoke npm somewhere.

@kanadgupta
Copy link
Member

Hey @joscha — this change is exclusive to the v7 channel (tracked via main). We have no plans to backport this to the v6 channel (tracked via v6). With the exception of critical security fixes, all current and upcoming feature work will be on the v7 channel.

@joscha
Copy link

joscha commented Aug 28, 2024

this change is exclusive to the v7 channel

Understood. It doesn't seem to be working as expected on 7.0.0-beta.8 for me as far as I can tell. See second part of the log above.

@kanadgupta
Copy link
Member

@joscha mind filing a new issue for this? Thanks!

@joscha
Copy link

joscha commented Sep 3, 2024

@joscha mind filing a new issue for this? Thanks!

Sorry, after there was no answer and I also read somewhere else in this repo that it doesn't support distribution, I moved to https://github.com/OpenAPITools/openapi-generator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Issues related to the `api` CLI, which builds the SDKs enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect the existing package manager during install
4 participants