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

feat: add nix-shell and direnv support #251

Merged
merged 1 commit into from
Aug 15, 2022
Merged

feat: add nix-shell and direnv support #251

merged 1 commit into from
Aug 15, 2022

Conversation

hall
Copy link
Collaborator

@hall hall commented Aug 13, 2022

Here are some misc changes that were helpful to me while developing; namely adding a flake devShell and direnv setup such that all the dependencies are automatically made available in the vscode session (if the direnv extension is installed).

@jnoortheen
Copy link
Collaborator

jnoortheen commented Aug 14, 2022

@hall it would be good to mention this in readme.
Also lock need not be included in the repo.

@hall
Copy link
Collaborator Author

hall commented Aug 14, 2022

Thanks @jnoortheen, I added some wording to the README.

Why not commit the lock file to the repo? Seems like that defeats the purpose but maybe there's something I'm missing.

@jnoortheen
Copy link
Collaborator

eventhough any nodejs version is supported, it might download an old copy as the time goes and nodejs moves onto represent latest version. It is better be left to users, otherwise we would need to update the lock file regularly.

@hall
Copy link
Collaborator Author

hall commented Aug 14, 2022

That's true but I just tested removing the lock file, adding flake.lock to .gitignore, and re-running nix develop still added it to the index so I'm not really sure how well keeping it out of the repo will go. Maybe we should add a note to the README about running nix flake update before starting development?

@jnoortheen
Copy link
Collaborator

jnoortheen commented Aug 14, 2022

you could add it to .gitignore list.
Edit: sorry let me check a bit.

@hall
Copy link
Collaborator Author

hall commented Aug 14, 2022

Yeah, let me know if your git-foo is better than mine; here's the quick test I did:

$ git ls-files flake.lock 
flake.lock

$ git rm flake.lock
rm 'flake.lock'

$ git ls-files flake.lock 

$ grep flake.lock .gitignore
flake.lock

$ nix develop
. . .

$ git ls-files flake.lock
flake.lock

Seems nix force-adds the lock file to the git index.

@jnoortheen
Copy link
Collaborator

seems like the issue is reported. NixOS/nix#5810
We should wait for it. In the meantime, anyone can refer this PR.

@hall
Copy link
Collaborator Author

hall commented Aug 14, 2022

Just barely beat you to it 🙂 doesn't look like there's any indication of intention to fix that issue.

Some alternatives:

  • use the nodejs-16_x package so the version is explicit and easier to match/bump with other files (e.g., node-version: 16 in CI or the "@types/node": "^18.7.3" dependency in package.json)
  • use shell.nix, instead of flakes, which pulls packages from the user's configured nixpkgs channel (which should only be done with the option above since that would pull in whatever version happened to be latest at the time).

I still think adding the lock file is the right approach; otherwise, every user (which is only contributors, not the entire set of extension users, to be sure) needs to pin/maintain their own dev environment. So maybe I don't understand, what are other contributors doing? Maintaining a shell.nix out of tree? Installing packages into their system environment?

@jnoortheen
Copy link
Collaborator

Yes shell.nix would be helpful. The node version is not a hard dependency and only used to compile the extension. We want something that gives maximum flexibility for the dependencies.

I, myself moved on from nixos and use nix packages rarely. I am not sure about others. I hope, the whole flakes thing need to be more user friendly.

@hall
Copy link
Collaborator Author

hall commented Aug 14, 2022

Cool, I removed the flake in lieu of using nix-shell. Let me know what you think.

Ah, OK. Let me know if you'd like help with maintenance if you're looking to spent less time on nix stuff; I don't have a ton of experience but have moved most of what I have to nixos at this point and am starting to work on getting more familiar through various projects.

@hall hall changed the title feat: add flake and direnv feat: add nix-shell and direnv support Aug 14, 2022
@hall
Copy link
Collaborator Author

hall commented Aug 14, 2022

Snuck a quite change in (where this PR was already making modifications) to make the settings.json snippet in the readme jsonc instead of json so it don't highlight comments in bright red.

@jnoortheen
Copy link
Collaborator

I removed the flake in lieu of using nix-shell

Looks good.

Let me know if you'd like help with maintenance

Sure, you can help me review the PRs. I think someone from nix-community has to add you to the repo. I will check it. Most of the heavy lifting is done by the rnix-lsp project, so the JS/typescript knowledge is enough for the project

@jnoortheen jnoortheen merged commit d78b62d into nix-community:main Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants