chore(build): add Prettier to the project, expand on #168

[ghiscoding]

Description

takes PR #168 and addresses all comments provided in the original PR

Motivation and Context

• fixed npm lockfile that got broken by original PR, I took the same approach as the pnpm lock file update to fix the npm lock file update. Both updates are now very similar and easier to read
• removed preferred-pm since I would prefer to not use any extra dependencies, and I think that also fixed the unit tests since they weren't being detected properly
• added a prettier VSCode setting (setup) and also fixed couple of places where prettier wasn't doing a good job
• add a new flag no-update-root-lock-file just in case this pnpm update causes issues, at least the user could disable it

How Has This Been Tested?

only local unit tests for now, we might need a bit more testing, need to validate with @JacobSoderblom

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[ghiscoding]

@JacobSoderblom
I took your original PR and addressed all comments I left the first review, I also fixed a bug that you introduced and might have missed with the npm lock file update that wasn't updating correctly anymore and is now working again (your original PR updated the unit test snapshot with wrong version number). When you have a chance, could you please review this PR? I created this new PR because I couldn't write directly to your branch (I actually don't know if I need both PRs, it seem to push on both branches anyway) and didn't hear from you but I still wanted to advance this PR since I'm also moving towards pnpm workspace and I'll need this as well 😉

Thanks for your contributions
Cheers

[codecov]

Codecov Report

Merging #182 (00ce3e0) into main (a01ab09) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #182      +/-   ##
==========================================
+ Coverage   93.29%   93.31%   +0.02%     
==========================================
  Files         134      134              
  Lines        3901     3911      +10     
  Branches      773      785      +12     
==========================================
+ Hits         3639     3649      +10     
  Misses        262      262              

Impacted Files	Coverage Δ
packages/core/src/utils/index.ts	100.00% <ø> (ø)
packages/version/src/index.ts	100.00% <ø> (ø)
...kages/cli/src/cli-commands/cli-version-commands.ts	94.34% <100.00%> (ø)
packages/core/src/utils/npm-conf.ts	100.00% <100.00%> (ø)
...ackages/version/src/lib/update-lockfile-version.ts	100.00% <100.00%> (ø)
packages/version/src/version-command.ts	99.73% <100.00%> (+0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de19859...00ce3e0. Read the comment docs.

[JacobSoderblom]

Looks good! 🥇

[JacobSoderblom]

I'll try to manage to test this locally today as well

[StarpTech]

Any update? This is also for me the only blocker to try out lerna-lite 😄

[ghiscoding]

@StarpTech it's currently in WIP, I don't think it will be until next week or even the week after, that I ship anything related to that... however you could easily use a temporary workaround for this via lifecycle script post to update your lock file, or use this other workaround shown in this post. Using the postversion with the workaround shown in the post is probably the easiest.... or you can wait a week or two until I can finish it. I just want to make sure not to ship something that might cause more problems than it resolves.

// preversion:    Run BEFORE bumping the package version.
// version:       Run AFTER bumping the package version, but BEFORE commit.
// postversion:   Run AFTER bumping the package version, and AFTER commit.

[StarpTech]

@ghiscoding thank you! The postversion way would run the update for every published package. Something like this could work pnpm install --filter='<package-name>' --lockfile-only

[ghiscoding]

@StarpTech @JacobSoderblom
After searching about the same thing after the workaround that I just posted, it seem that Changesets has the same issue which is still open and funny enough the pnpm maintainer (zkochan) is actually suggesting the exact same thing. I actually wonder if it wouldn't be easier to just call pnpm install --lockfile-only internally instead of trying to trying to modify the lock file (mostly because we never know when they'll decide for whatever reason to change their lock file structure), it should use the correct syntax depending on what the user set as his npmClient in lerna.json and after the install is done then just push it to the git changes list like it currently does. @StarpTech Are there any particular reason you've add the package name as a filter? The other thing that worries me with current PR is that calling writeYamlFile seems to change the file structure by removing all extra lines (it might be because of Prettier or something else) and the developers (you) will see a lot of white space changes the next time you'll run a pnpm install, which is another reason that running pnpm install --lockfile-only internally might be a better approach and will be much faster to implement (maybe 1 day versus a week or more), but again I'd like to know why you filtered by a package name?

[StarpTech]

Are there any particular reason you've add the package name as a filter?

Yes! If I don't scope it to a single package pnpm might throw an error that another workspace package refers to an invalid version. You can only run pnpm install --lockfile-only after all packages have been bumped. I recommend you to call internally pnpm install --lockfile-only once. It is also required to add the lock modification to git git add ../../pnpm-lock.yaml.

[ghiscoding]

I gave a try with npm i --package-lock-only directly in Lerna-Lite (with --git-dry-run) and I seem to have issues because running this command does search on npm registry and doesn't find the newer versions because it's not yet published (that is when we run lerna version), so this strategy might not work. The only way that this would work, would be to run this command after lerna publish which is much later in the process and I'm not sure that everyone would do both version and publish every time, if they only run version then the lock file would not be updated.. that might be ok though!? I'm not sure...

I'll still look at directly updating the lock file, so perhaps if I get both working then we could also have flags for both ways and let the developer choose.

npm ERR! code ETARGET
npm ERR! notarget No matching version found for @lerna-lite/init@^1.4.1.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

So basically the error comes from trying to retrieve "@lerna-lite/init": "^1.4.1" from the devDependencies however the package version number "version": "1.4.1" inside the same package.json doesn't throw the error.

{ 
   "name": "@lerna-lite/cli",
   "version": "1.4.1"                   // this line is ok
   "devDependencies": { 
     "@lerna-lite/init": "^1.4.1" // this line throws, not found in registry
  }
}

[StarpTech]

Did you use npm or pnpm?

[ghiscoding]

that was with npm on Lerna-Lite project itself, but I imagine pnpm will do something similar, isn't it?

[StarpTech]

No, pnpm should really just update the lock 😄

[StarpTech]

According to npm docs:

The --package-lock-only argument will only update the package-lock.json, instead of checking node_modules and downloading dependencies.

Maybe you need to operate in the workspace? https://docs.npmjs.com/cli/v8/commands/npm-install#workspace

Looks like a bug or it's not the whole truth.

[ghiscoding]

It won't download any packages but it will check if they exist in the registry, perhaps that part is not mentioned which is probably the most significant lol. You're saying it doesn't do that with pnpm, could you double-check?

So what I could do is add a flag in the publish command to update the lock file and do an extra git commit for this instead of currently being in the version command with a single git commit, but I'm not sure everyone will want to publish right away and get they lock file updated with a separate commit.

[StarpTech]

could you double-check?

Yes, but it takes some time.

It won't download any packages but it will check if they exist in the registry, perhaps that part is not mentioned which is probably the most significant lol.

Did you also try npm install --workspaces --offline --package-lock-only

So what I could do is add a flag in the publish command to update the lock file and do an extra git commit for this instead of currently being in the version command with a single git commit, but I'm not sure everyone will want to publish right away and get they lock file updated with a separate commit.

In my opinion, updating the lock file should be part of the versioning commit because it belongs to the "versioning" process to ensure integrity. My use case is to version and publish together.

[ghiscoding]

ahh I didn't know about that offline option, dang there's so many flags... however after trying it over lunch, it doesn't seem to help and I'm pretty much getting the same error (with/without --workspaces is the same too), kinda weird

> npm install --workspaces --offline --package-lock-only
npm ERR! code ETARGET
npm ERR! notarget No matching version found for @lerna-lite/init@^1.4.1.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

ahhh I think it's because of what they say in the description

If true, staleness checks for cached data will be bypassed, but missing data will be requested from the server. To force full offline mode, use --offline.

ohh but wait I found another command shrinkwrap and it does run without any error but that command has the side effect of deleting the lock file and renames it to npm-shrinkwrap.json, if however I copy its content and put back in place the package-lock.json file with that content, it does the bump. Perhaps they have a way to bypass the delete/rename of the lock file.. or I could run that command and rename the file, but not exactly straightforward (but actually shown in this Stack Overflow answer, perhaps it might be ok, I just wonder how pnpm and yarn would handle something like that)

This command repurposes package-lock.json into a publishable npm-shrinkwrap.json or simply creates a new one.

and this other description package-lock.json vs npm-shrinkwrap.json

[StarpTech]

PNPM behaves correctly when all version constraints can be resolved with the local packages.

[StarpTech]

@ghiscoding Did you try it with a pnpm monorepo? It must work.

[StarpTech]

So finally. I tried it on a fresh pnpm monorepo with two packages. No errors. The local versions are used before looking at the registry.
test.zip

[ghiscoding]

@StarpTech no I only tried with Lerna-Lite itself, I'd like to have a generic solution that works for all type (if possible). The approach might be different on each manager and that's ok too, if I do add that option it would be under a new flag that is disabled (opt-in), something along --install-lockfile-only. I found this post just now for npm, I'll give it a try tonight and see if that fixes npm, seems like pnpm is ok and I have no clue about yarn yet.

On the other hand, I'm still looking at updating the file itself, I found last night that pnpm maintainer is using its own custom version of js-yaml in this PR to write their pnpm lock file with extra empty lines between groups, so at least I figured that out, because if I do provide that option to update the pnpm lock file directly I wish to keep its structure the same, else developers will for surely complain. This would also be under an opt-in flag, something along --update-root-lockfile

and thanks for the quick project repro, this will be helpful

[StarpTech]

As a lerna user, you have to specify npmClient. That means you can provide that fix just for pnpm. I don't recommend playing "package manager" because it's complicated and error-prone.

[ghiscoding]

As a lerna user, you have to specify npmClient. That means you can provide that fix just for pnpm. I don't recommend playing "package manager" because it's complicated and error-prone.

yes the intention is to add a switch case on npmClient for each package manager for that new flag. Internally it would run that lock only command for whatever client you chose and then do a git commit + push, so that it's less work for the developer (you) to use.

[StarpTech]

Sounds good! Would be happy to review your PR.

[ghiscoding]

@StarpTech I have a draft PR for the package manager client to update the lock file, I only tried within Lerna-Lite itself in dry-run, I did not try pnpm yet (neither yarn) but if you could take a look at the draft PR and provide any feedback then go ahead to PR #199

Note that I'm not sure if I'll keep working on modifying the pnpm lock file ourselves or just use that new PR to do the job, I think that I might keep both flag for at least npm but not sure about pnpm, if this new approach works better and is more reliable then I will just keep 1 approach. I'm not exactly sure how I'll unit test that though

[StarpTech]

@ghiscoding I have a repro where NPM works fine like pnpm. Again as long as the version constraint match with the local packages, no error is thrown
test-npm.zip. Could you share a reproducible example with your error?

And according to npm/cli#3637 only npm@>=8.5.0 implemented it properly.

[ghiscoding]

@StarpTech
hmm I might not have that npm version installed then, but the repro is simply Lerna-Lite itself, you can clone it and git checkout the draft PR and that is my repro... yeah just checked and I have npm 8.4.1, this npm fix is not very old either. So I could maybe use shrinkwrap when older than 8.5.0 or use regular install on newer npm version. I'll give that a try later after work

[ghiscoding]

just for reference, since PR #199 got merged and is a better way of handling the pnpm lockfile update. I decided to transform the original PR #168 from @JacobSoderblom and keep only the code that adds Prettier to the project. This way I get to keep his great contributions to the project and his effort and contribution are not entirely lost. Thanks for this great contribution.

I'm expecting to do a release in the coming 1 or 2 days.