Regarding cloning repo for the 1st time: README may be incorrect/outdated
Closed this issue · 9 comments
Problem spinning up the repo
I tried to spin up the repo and followed the README with the steps mentioned.
I run into some issues where pnpm was not happy with the configuration
The docs suggest using ni to just use the right tool automatically, but all my problems were solved when simply using pnpm.
Tangent: pnpm
To update pnpm to version 9, I ran this
pnpm add -g pnpm@9since the suggested pnpm self-update simply returns an error, couldn't figure out why.
Actual working flow
So, after downloading the repo and cding into it, you can
nvm i # to install the correct node version
corepack enable
pnpm add -g pnpm@9 # ensure your pnpm is version 9
pnpm i
pnpm devDisclaimer
I'm new to this environment so this information may be wrong/out of order. Please suggest any edits if you see errors!
Leaving this here for future reference. Thanks so much!
If we want to update the docs to only suggest pnpm, I have opened a PR to implement these changes here: #20
But maybe we want to try and fix ni installation instead, I don't know enough about how it works to try it myself though.
It looks like the first command has an extra argument:
- ni dev
+ ni(ni dev is equivalent to npm i dev (installing a package named dev), which caused the above error)
dev is an unnecessary part here.
And dev should be run with nr as a separate command as shown in the ni's README (https://github.com/antfu-collective/ni#ni):
nr devSo these lines should be correct.
If we improve the instruction, I saw several ones were able to solve the issue by removing node_modules/ directory and retrying the instructions. Maybe we can suggest that operation when developers encounter an issue during the initial setup.
ohh that makes a lot of sense. then it seems it's not broken at all, just a missing nr dev in the readme?
So I just checked in the whole repo, and the dev command is in fact correct in all the README and doc.
However I agree that we can include some steps to help when the setup is not working, and to maybe point out the nivs nrdifference in case it was missed, so I opened a new PR to implement this : #21
This is an alternative to #20, and in my opinion the better one. @jesi-rgb let me know if #21 makes sense to you and if it would be enough to understand what to do to solve the issue. 😊
@IonianPlayboy Just my two cents here:
The repository is a pnpm monorepo, and as such pnpm is required no matter what. ni is an abstraction on top of this that is useful for when you just want to use one set of commands across different projects such as yarn, npm, pnpm. In my experience, it's better to only mention one (the happy path). In this case, we should stick to pnpm.
My thinking on this is that people who know about and use ni already understand how it functions and what they need to do to get it working with different projects. New users, particularly those unfamiliar with the JS ecosystem, will probably just want to follow an exact step-by-step with no branching paths.
For this reason, I'm in favor of dropping all mentions of ni and nr and only documenting pnpm.
I agree with @Sporiff here (and others that said the same before). Let's drop mentions of ni. If we're already going to show pnpm, that's enough, there is no need to show both. We wanted to promote the use of ni in Elk, just to show something cool to others. Having pnpm is equally good, let's promote pnpm :)
The PR has been reopened and updated:
Done in #20