feat(nix): Add development shell and package export#231
feat(nix): Add development shell and package export#231elucid merged 17 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
86c9929 to
8aafa38
Compare
|
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 |
- 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.
Restore the generated Nix dependency lock to match the existing root bun.lock instead of carrying an unrelated npm Bun package bump. The flake build should solve its Bun selection inside Nix packaging without changing Hunk's published npm fallback runtime dependencies.
Pass pkgs.bun into the package derivation and invoke it through an explicit store path. This prevents the compile step from resolving a Bun binary out of bun2nix's installed node_modules tree.
Remove bun2nix from the contributor shell inputs and stop passing it through flake.nix. The shell only needs Bun, Node.js, and Git, and avoiding the bun2nix package keeps nix flake check --no-build from realizing bun2nix internals.
Document that users update Hunk through their own flake.lock pins and that maintainers regenerate nix/bun.lock.nix explicitly when JS or Bun dependencies change.
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.
Closes #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.