-
Notifications
You must be signed in to change notification settings - Fork 3
chore: migrated to package manager to pnpm #204
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 51df6e3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This is even showing errors that does not show with npm.
|
I didn't experience that when I cloned and installed the repo. Would it help if we just regenerated the What version of |
Already did. Same version. With pnpm another issue appeared through the CI, and were not showing before with npm. Clearly pnpm are working properly where npm was not.
|
…replace-package-manager
And more.
|
Can you review? In some fixes, you should consider the comments I made. It's related. |
@@ -88,6 +88,7 @@ | |||
"devDependencies": { | |||
"@babel/core": "^7.26.10", | |||
"@babel/types": "^7.27.0", | |||
"@types/babel__core": "^7.20.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was this fixing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!existsSync(dir)) { | ||
return results; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks an invariant and shouldn't fail silently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you prefer to be done?
if (!fs.existsSync(archDir)) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks an invariant and shouldn't fail silently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you prefer to be done?
if (!fs.existsSync(archDir)) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks an invariant and shouldn't fail silently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you prefer to be done?
I would like for you to create an issue for the NPM issue which prompted you to want to migrate to PNPM in the first place, I'm fairly certain we could fix that without migrating package manager. Some of the issues you're seeing might come from:
|
https://pnpm.io/cli/recursive#--no-sort Weird that you do not prefer better tooling. As one can easily replace one for another, if ever needed. I gave the reasons in the PR. Not looking to resolve the issues in the current, but actual improve while resolving some that I mentioned. pnpm are clearly more functional than npm. |
Not weird at all - we're the ones maintaining this package in the long run and choosing technology / tools aren't soly about choosing the seemingly superior solution in my experience. To be clear, I'm not saying that NPM is flawless nor that we won't make a switch to a different package manager and I'm not looking to get more into that conversation right now.
I disagree, as I'm unable to reproduce the issue: You're not providing specific commands and outputs from running those and therefore I'm unable to get a strong signal, giving me a compelling reason to accept your suggested change. |
Of course. I did provide, there should be plenty of commands explaining in the first post and then in the ones that was triggered in the CI. I meant weird in a sense where it's a just a simple tool that seem to be doing a bit more than the current. As any can be easily replaced by another. Also in a sense where it's not a life decision where it could lead you to a path or another, due to having made the decision of changing a tool. |
I added the Now it should be the same. I answered your reviews done in here. I have updated the first description post, it should be more clear. In my experience, pnpm (or yarn if not possible) and biome are minimum in a project. If after all the information provided, it's stil not enough, it's a waste, but feel free to close the PR. |
Another issue appeared here. Build from ferric were not being generated. I then removed But now the error are this one. And nothing to fix.
Then again, I installed with pnpm. It found the errors I mentioned in this PR with babel, where before with npm they dont even appear.
Wow, and the build now works with pnpm. |
Are you saying that regenerating the lockfile led to npm working for you? |
No, with that, it made it worse, it resulted in the error mentioned. Currently, I am using pnpm as workaround without commiting it, to be able to properly run the project. |
Before I had trouble using
npm i
in the root, ferric-cli were not running, it wasnt finding the js.Even if it were recognized in the package-lock as workspace installed.
It only worked after I did a
pnpm i
straight in the ferric folder, then I was able to run the command standalone. That's the experience I had.This led to this PR. Which in fact work now just after a
pnpm install
in the root folder.pnpm
are faster, in my experience, there is no reason to not migrate to it, even more if fromnpm
.https://pnpm.io/pnpm-vs-npm
https://refine.dev/blog/pnpm-vs-npm-and-yarn/ - Faster CI installs and more