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

Add a buildPnpmPackage ? Or add pnpm build and install hooks? #317927

Open
doronbehar opened this issue Jun 7, 2024 · 8 comments
Open

Add a buildPnpmPackage ? Or add pnpm build and install hooks? #317927

doronbehar opened this issue Jun 7, 2024 · 8 comments

Comments

@doronbehar
Copy link
Contributor

doronbehar commented Jun 7, 2024

Continuing from:

Which was fixed by:

This issue is dedicated to discussing whether we want a buildPnpmPackage nix function to be defined, or perhaps we want to have pnpm.buildHook and pnpm.installHook defined (along with pnpm.configHook which was added in #290715 ).

Currently, it seems that npmHooks.npmBuildHook and npmHooks.npmInstallHook which do a similar job, only they use npm run build and not pnpm run build. If eventually we'll decide we don't need new hooks for pnpm, we should at least document that npmHooks are a good alternative.

There are some issues with pnpm deploy as mentioned by @NyCodeGHG in #231513 (comment) , so this should be taken in mind.

Another detail worth mentioning, is that for bash-language-server, which uses pnpm workspaces, the docs are slightly in contrast to what works for that package, see #333701 (comment) .

ccing other people involved: @Lord-Valen @dhess @teto .

@GetPsyched
Copy link
Member

I'm not as aware about the technical side of this as you guys, but here's what I think:

In nixpkgs, we have all sorts of helper functions specific to languages. We also have one specific to tooling, buildNpmPackage. Unless the idea of bundler-specific helper functions itself is contested, I think we should do the same with any bundler that's to be supported, pnpm, yarn, bun, deno, etc. (for the record, I'm not saying we should support all but rather the idea of consistency).

Since buildNpmPackage exists, any package maintainer would reasonably expect there to be one for pnpm et al. For that reason alone, I think it's better to have a buildPnpmPackage than more hooks. (it also makes the derivation verbose for no benefit that I can see at least)

@eclairevoyant eclairevoyant changed the title Add a buildPnmpPackage ? Or add pnpm build and install hooks? Add a buildPnpmPackage ? Or add pnpm build and install hooks? Jun 7, 2024
@pluiedev
Copy link
Contributor

pluiedev commented Jun 7, 2024

I think the steps required to build a pnpm-based package are already convoluted enough to warrant a wrapper 😁 We can certainly expose those hooks for packages that need more fine-grained control, but we should have an equivalent of buildNpmPackage for simple use cases

@NyCodeGHG
Copy link
Member

i don't think anything speaks against having a buildPnpmPackage function as long as the hooks are also usable in more complex scenarios outside of buildPnpmPackage (just like with the current hooks for npm)

also maybe pnpm.buildPackage would be a better name and more consistent with pnpm.fetchDeps?
just a suggestion let me know what you think

@pluiedev
Copy link
Contributor

pluiedev commented Jun 7, 2024

buildPackage, mkPackage, it doesn't really matter honestly, but it does have to be under pnpm_{8,9} since the hooks all depend on that (breaking lockfile changes and what not)

@Eveeifyeve
Copy link
Contributor

Eveeifyeve commented Jul 19, 2024

Here are my thoughts on adding another builder,
I think there should be one builder that has the support for deno, bun, yarn, and etc called buildNodePackage as adding another builder is kinda adding a builder when instead you could combine it into one and keep it simple(kiss rule). but this would be good as if there is a new package manager you can just add support into the builder rather than creating a whole builder.

@Eveeifyeve
Copy link
Contributor

Eveeifyeve commented Aug 7, 2024

Here are my thoughts on adding another builder, I think there should be one builder that has the support for deno, bun, yarn, and etc called buildNodePackage as adding another builder is kinda adding a builder when instead you could combine it into one and keep it simple(kiss rule). but this would be good as if there is a new package manager you can just add support into the builder rather than creating a whole builder.

And for people who don't want to use the builder and want to fetch deps what if you introduce fetchNodeDeps which as above has support for all the package managers which fetches deps and not create a whole fetcher.

@Eveeifyeve
Copy link
Contributor

Also a hook would be much better as adding a builder doesn't give you the modifications from buildNpmPackage or something an example is at #335751

@getchoo
Copy link
Member

getchoo commented Aug 26, 2024

The best solution would probably be to have both hooks and a builder function that uses them -- buildNpmPackage does this for example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants