feat(nix): Add development shell and package export#231
feat(nix): Add development shell and package export#231m4r1vs wants to merge 13 commits intomodem-dev:mainfrom
Conversation
Greptile SummaryThis PR adds Nix flake support to the repository, providing a
Confidence Score: 3/5The Nix infrastructure changes are non-breaking for existing npm/bun workflows, but the Two issues block the stated goals:
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User runs `bun install`] --> B[postinstall hook fires]
B --> C[bun2nix -o nix/bun.lock.nix -c ../]
C -->|Wrong path flag| D[⚠️ Context = parent of repo root]
C -->|Overwrites for all devs| E[nix/bun.lock.nix regenerated]
F[User runs `nix develop`] --> G{Look for devShells.sys.default}
G -->|Not found - deprecated devShell.sys| H[⚠️ May fail on newer Nix]
I[.envrc use flake] --> G
J[User runs `nix build .#packages.sys.default`] --> K[packages.sys.default]
K --> L[nix/package.nix bun2nix.mkDerivation]
L --> M[bun build --compile src/main.tsx]
M --> N[result/bin/hunk OK]
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
flake.nix:42-51
**`devShell` should be `devShells`**
The modern Nix flake schema uses `devShells.${system}.default`, not `devShell.${system}`. With the current structure, `nix develop` and `use flake` in `.envrc` will fail to resolve the dev shell because they look for `devShells.${system}.default` — the top-level `devShell` attribute is the deprecated singular form. The `packages` output already uses the correct nested format; `devShells` needs the same treatment.
```suggestion
devShells = forAllSystems (
system: let
pkgs = import nixpkgs {
inherit system;
};
in {
default = pkgs.callPackage ./nix/devShell.nix {
inherit pkgs;
};
}
);
```
### Issue 2 of 4
package.json:73
**`postinstall` regenerates Nix lockfile for every developer**
The `postinstall` script runs `bun2nix` on every `bun install`, meaning every contributor will silently overwrite `nix/bun.lock.nix` even if they have no interest in Nix. Additionally, the `-c ../` flag points one directory above the repo root when `postinstall` runs from the package root, which is almost certainly wrong. Consider moving this to a standalone script or an explicit `npm run nix:lock` command instead.
```suggestion
"nix:lock": "bun2nix -o nix/bun.lock.nix"
```
### Issue 3 of 4
nix/package.nix:1-4
**Version hardcoded and will drift from `package.json`**
`version = "0.10.0"` is duplicated from `package.json`. When the project version is bumped, this derivation will silently report the wrong version. Consider reading it from the source tree via `lib.importJSON ../package.json`.
```suggestion
{ bun2nix, lib, ... }:
let
packageJson = lib.importJSON ../package.json;
in
bun2nix.mkDerivation {
pname = "hunkdiff";
version = packageJson.version;
```
### Issue 4 of 4
nix/README.md:20-23
**`stdenv` is likely out of scope in this snippet**
The example uses `${stdenv.hostPlatform.system}` without a `pkgs.` prefix. In a typical NixOS or home-manager module, `stdenv` is not automatically in scope — the reader would need `${pkgs.stdenv.hostPlatform.system}` or the simpler `${pkgs.system}`. As written, copy-pasting this example will result in an "undefined variable" evaluation error.
Reviews (1): Last reviewed commit: "fix: remove bun as npm dependency" | Re-trigger Greptile |
162c5ca to
bf6ccca
Compare
3e5012d to
cad81ad
Compare
- Define a development shell in ./nix/devShell.nix - Use nix-community/bun2nix to package hunk
Having bun install itself was causing weird issues. I've never seen it installed as a nodjs package anywhere else so I went ahead and removed it.
|
Hey @m4r1vs (cc @emrtnn). Thanks for the PRs and for the discussion. I've looked at both this and #230. #230's approach is simpler but as you've both discovered, the fixed node_modules hash is platform dependent. I think this PR's approach is more nix-native and more maintainable. Having said that, you'll need to make some fixes before it can be merged:
|
|
Thanks for the feedback! Appreciate it!
|
|
regarding 3) I think it does not run them by default (see here) but I'll make it explicit |
|
@elucid I think I've been able to address all your points:
Cheers! I think the removal of postinstall bun2nix should also fix the failed pipeline |
|
I've noticed that A more idiomatic way would be to copy it to $out/share but that would mean, we'd have to modify |
I use nix and try to avoid globally installing npm packages.
There is a pull request to add Hunk to nixpkgs upstream,
however it is stalled by support for direct bun packaging.We use bun2nix which I have had good experience with in the past.
This would close #215
I've tested this on x86 NixOS, arm64 Fedora with Nix installed and MacOS Tahoe with nix-darwin.
I ran into a couple of issues which I solved by removingbunas a dependency inpackage.json. Not sure why it's there -- I've never seen it installed like this before.I tried to keep the documentation in README and CONTRIBUTING minimal and elaborated more in a new README in the
nixdirectory.Happy to answer any questions and to maintain the nix flake if maintanance is needed. Cheers :)
Ps. I also added a home-manager module so Hunk can be configured using nix. It also allows setting it as the default git pager so
git diffworks when using hunk through home-manager without any extra configuration needed.