fix: robustness improvements for shell hook #544
Conversation
Deploying with
|
| Latest commit: |
ffaf563
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://31b38337.primer-app.pages.dev |
| Branch Preview URL: | https://brprice-robust-shell-hook.primer-app.pages.dev |
README.md
Outdated
|
|
||
| After the initial bootstrap, you'll be able to run just `nix develop` and then all the standard `pnpm` command should work. | ||
| Then you'll be able to run just `nix develop` and then all the standard `pnpm` command should work. |
There was a problem hiding this comment.
IMO, this sentence reads slightly awkwardly with the repetition of "then".
There was a problem hiding this comment.
Thanks. Removed the second "then".
brprice
force-pushed
the
brprice/robust-shell-hook
branch
from
ffaf563 to
cb49712
Compare
| @@ -35,9 +35,10 @@ Note that, in this project, we do *not* use Nix in any significant capacity. Nix | |||
|
|
|||
| ## Interactive development | |||
|
|
|||
| To develop interactively, enter the Nix shell via `nix develop`. Note that due to https://github.com/NixOS/nixpkgs/issues/132456, we cannot use Nix to install `pnpm` with any version of Node.js > 14, so the first time you check out the `primer-app` repo, you'll need to run `npx pnpm install` to bootstrap a new repo. | |||
| To develop interactively, enter the Nix shell via `nix develop`. Note that due to https://github.com/NixOS/nixpkgs/issues/132456, we cannot use Nix to install `pnpm` with any version of Node.js > 14, so the first time you check out the `primer-app` repo, you'll need to run `npx pnpm install` (in a `nix develop` shell) to bootstrap a new repo. | |||
There was a problem hiding this comment.
We might as well drop the notice about NixOS/nixpkgs#132456. My reasons for wanting to avoid using Nix to get pnpm stand, but that issue is not actually a blocker anymore. So we could instead say something like, "The first time you check out the primer-app repo, you'll need to run npx pnpm install (in a nix develop shell) to bootstrap."
| ln -s ${spec} ${local-spec} | ||
| cd packages/primer-app && pnpm generate && cd - | ||
| if ! type -P pnpm ; then | ||
| echo "'pnpm' not found: there is some one-time setup required, see README.md" |
There was a problem hiding this comment.
Why not just run npx pnpm install here and then tell the user to exit and re-enter the nix develop shell?
There was a problem hiding this comment.
I assumed you intentionally had not done that previously. I'll change (in fact, I think we can avoid having to exit and re-enter in that case)
|
Let's hold off on merging this until the project refactoring work is done. |
|
Closing in favor of #590. |
We now bail out early with a helpful message if
pnpmis not available (i.e. because bootstrapping has not been done), and always return to the root directory even ifpnpm generatefails.