-
-
Notifications
You must be signed in to change notification settings - Fork 6k
Switch to pnpm #35274
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
Switch to pnpm #35274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnpm is awesome - but addoption is not to hight so support in other tools is till kinda lacking
-> plz ask more devs for lgtm ... as they also have to work with it :)
@techknowlogick would be nice if you could create a pull upstream
My drive welcomes this as well :)
-> plz ask more devs for lgtm ... as they also have to work with it :)
@6543 yup, that's why I also requested a review from @silverwind as they have a lot of interaction with the js side of things so I don't want to negatively impact them just for a slight improvement for me.
I requested a review from you as I know you use it with woodpecker already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm neither for nor against it.
I lack the experience with it to judge if pnpm is a good choice or not.
Nevertheless, there seems be a clear opinion from the TOC in favor of it.
Still, it's probably best to wait for approval from the people that are most impacted by it (@silverwind and @wxiaoguang).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested it. I am neutral for it.
Signed-off-by: techknowlogick <matti@mdranta.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnpm is awesome not only because of the linking, but also because of the easily diff-able package lock file.
This adds a new build dependency, so the build docs need to be updated to mention this requirement:
https://github.com/go-gitea/gitea#building
As for the change, I'm -0 because I see no need to change this package manager. gitea's node_modules consumes 629MB currently. Hard linking npm dependencies creates problems when you want to edit a dependency for debugging and has problems across file system boundaries like docker volumes. I see no benefit.
I also had to fork the license checker to support analyzing pnpm projects.
I'd rather we drop the license.txt generation completely, it's been causing too much issues.
Co-authored-by: silverwind <me@silverwind.io> Signed-off-by: techknowlogick <matti@mdranta.net>
I guess you can merge this now and we remove the license.txt generation later (along with your fork), if we get consensus on that removal.
Makefile
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnpm install does not have a --no-save option: https://pnpm.io/cli/install and it therefor errors:
ERR_PNPM_OPTION_NOT_SUPPORTED The "add" command currently does not support the no-save option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently there is open issue pnpm/pnpm#1237 about it. I'm not sure how to solve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what we should do is make a second package.json and lockfile inside the tools directory that holds these dependencies. That way we avoid any pnpm install failures related to those native modules on the main package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I separated it out, and added cairo and pixman to the flake so that at least those who use nix can ensure they have the expected build deps. I'll have to add a mention of those two to the docs as turns out that prebuilt versions of canvas aren't available for at least my combination of devices and node versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I think there might be some wasm-based svg-to-png solution somewhere which would work everywhere, but this is good enough for now.
Edit: Likely https://github.com/ssssota/svg2png-wasm
361e59f
into
go-gitea:main
* giteaofficial/main: Switch to pnpm (go-gitea#35274) Switch `bitnami` images to `bitnamilegacy` on CI (go-gitea#35402) Upgrade dependencies (go-gitea#35384) [skip ci] Updated translations via Crowdin # Conflicts: # package-lock.json
opening this for discussion.
I use git worktrees to work on various PRs for Gitea, leading me to have sometimes up to 20different copies of node_modules installed. This takes a toll on the available space on my hard drive. pnpm uses hardlinks&symlinks to alleviate this issue.
I understand this takes us a step away from core nodejs tools, but I think for anyone who works on several branches at a time it would benefit them.
I also had to fork the license checker to support analyzing pnpm projects.