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 pnpm #91

Merged
merged 3 commits into from
Nov 24, 2021
Merged

feat: support pnpm #91

merged 3 commits into from
Nov 24, 2021

Conversation

mtoohey31
Copy link
Contributor

@mtoohey31 mtoohey31 commented Nov 6, 2021

Hello! This PR adds support for pnpm, I believe I've covered all the places where changes are necessary but let me know if I missed anything. I've tested every command except change_version because I'm running into issues with that, I believe they're unrelated because it looks like it's creating two version select windows or something like that, not sure, but more research is required before I open an issue for that.

Closes #76. As I mentioned in that issue, we can just use npm outdated --json to check outdated, since pnpm doesn't support it yet, but we should keep an eye on the issue in the pnpm repo and probably allow it to use pnpm when it's supported.

@mtoohey31
Copy link
Contributor Author

mtoohey31 commented Nov 6, 2021

Oh also, do you want me to add pnpm to the docs everywhere, or just to the parts where it explains what command it runs?

@vuki656
Copy link
Owner

vuki656 commented Nov 6, 2021

Really cool, thanks for the contribution.

Please do add docs so people know it's supported.

Regarding the change_version, can you describe a bit more of what happens?

As for the pnpm --outdated --json, after this is merged, I'll open an issue to track it so once pnpm supports it we can switch.

@mtoohey31
Copy link
Contributor Author

Please do add docs so people know it's supported.

Will do!

Regarding the change_version, can you describe a bit more of what happens?

Here's a screenshot:
2021-11-06T12:07:36,005557426-04:00

After I select the result in either window, I get the following error:

E5108: Error executing lua Vim:E475: Invalid argument: expected String or List

As for the pnpm --outdated --json, after this is merged, I'll open an issue to track it so once pnpm supports it we can switch.

Sounds good!

@mtoohey31
Copy link
Contributor Author

Actually, my bad, it turns out I did make a mistake and the error resulting after selecting either window was a bug in my new code for pnpm, so I'll fix that. I'm pretty sure the dual windows is another unrelated issue though, because it also happens in npm projects. I'll look into it!

@mtoohey31
Copy link
Contributor Author

Ok, pnpm-related issues are fixed, and docs are updated! Like I said, I'll try and look into the duplicate windows issue, but I believe it's unrelated to these changes since it also happens with npm.

@vuki656
Copy link
Owner

vuki656 commented Nov 7, 2021

Ok, pnpm-related issues are fixed, and docs are updated! Like I said, I'll try and look into the duplicate windows issue, but I believe it's unrelated to these changes since it also happens with npm.

Does that happen every time when you open the change_version popup?

@mtoohey31
Copy link
Contributor Author

mtoohey31 commented Nov 9, 2021

My appologies, the double window bug was a result of a failed attempt on my part to solve #86, as you can see by my comment on your issue, I had completely misunderstood what that check regarding the value being "" was doing. I believe I've come up with a better fix though, so maybe we can take a look at that in a separate PR, but afaik this one is good to go.

Sorry again, I should've been working on that on a separate branch instead of trying to do multiple things at once, lesson learned.

@vuki656
Copy link
Owner

vuki656 commented Nov 9, 2021

No problemo :)

I'm a bit busy at the moment but feel free to open a PR and I'll take a look within a few days.

@mtoohey31
Copy link
Contributor Author

No worries, it's open at #92 when you have time.

@vuki656
Copy link
Owner

vuki656 commented Nov 13, 2021

Hey, I just added some tests for command retrieval and loading status. Pretty straightforward stuff.

Could you please merge master into your branch and add tests for pnpm command retrieval. Let me know if you need any help with that.

@mtoohey31
Copy link
Contributor Author

Sure, that's done and seems to be working.

@vuki656
Copy link
Owner

vuki656 commented Nov 24, 2021

Just merged #92, is this ready as well?

@mtoohey31
Copy link
Contributor Author

I believe so! I've been using it since I wrote it, and haven't run into any more issues.

@vuki656
Copy link
Owner

vuki656 commented Nov 24, 2021

I believe so! I've been using it since I wrote it, and haven't run into any more issues.

Cool, thanks for the PR.

@vuki656 vuki656 merged commit fa98e8b into vuki656:master Nov 24, 2021
@vuki656 vuki656 self-assigned this Jan 4, 2022
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.

[FEATURE REQUEST] Support for pnpm
2 participants