svanderburg / node2nix

Generate Nix expressions to build NPM packages
MIT License
527 stars 100 forks source link

Extract common logic from composePackage to a shell function #255

Closed thefloweringash closed 3 years ago

thefloweringash commented 3 years ago

The installation logic is fairly generic, and only needs to know the packageName and src. Using a shell function instead of interpolating the variables into a repeated snippet reduces the derivation size significantly.

Also factor out some repeated logic in includeDependencies.

As a motivating example, these two changes combined reduce the derivation size of zigbee2mqtt from 3.2M to 690K.


Created in response to https://github.com/svanderburg/node2nix/issues/46#issuecomment-801378717

thefloweringash commented 3 years ago

I believe this is a fairly straightforward refactor that will help with upgrading zigbee2mqtt in nixpkgs. Would you have time to take a look? (polite bump)

svanderburg commented 3 years ago

I'll see if I can review this very soon.

I've also been working re-architecting several parts of node2nix, so that some big structural issues can be fixed properly. Unfortuntely, we've been running into many design limitations of node2nix, and the only way to cope with that is by fixing the architecture.

I've also been working on a major revision of node-env.nix by delegating most of its responsibilities to a companion tool called: placebo-npm. Using this tool should also fix the recursion problems and making the implementation of node-env.nix really simple, so that we can fix overriding issues etc.

Unfortunately, the integration is not done yet, and will probably still take a bit of time.

In the meantime, maybe this can go in until I have the integration ready.

svanderburg commented 3 years ago

Ok I'll get this in for now, until I have the replacement node-env.nix that uses placebo-npm finished.

thefloweringash commented 3 years ago

Thanks for the merge! I got a polite nudge from hexa on matrix to apply this to zigbee2mqtt in nixpkgs. Is there any chance of a release including this change? I can do the backport in nixpkgs, but I think a release would be cleaner.

svanderburg commented 3 years ago

Yes, I will do a release very soon. There are more things I need to integrate and I need to do some more testing....

On Tue, Oct 19, 2021 at 4:21 PM Andrew Childs @.***> wrote:

Thanks for the merge! I got a polite nudge from hexa on matrix to apply this to zigbee2mqtt in nixpkgs. Is there any chance of a release including this change? I can do the backport in nixpkgs, but I think a release would be cleaner.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/svanderburg/node2nix/pull/255#issuecomment-946775557, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIZR57BSBVVIZX6DKVVCKDUHV5IPANCNFSM5CJ34JBQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.