syncpack icon indicating copy to clipboard operation
syncpack copied to clipboard

pnpm workspace protocol not working with syncpack (workspace:*)

Open LudovicSterlin opened this issue 3 years ago • 4 comments
trafficstars

Description

pnpm support workspace protocol (workspace:). I'm using it in my monnorepo with a shared package and other packages that depend on it. When I try to use syncpack in my monorepo it raises erro on the workspace:* version.

This repo LudovicSterlin/pnpm-syncpack is a minimal working example for this issue.

When running

syncpack list-mismatches

You get the error below:

- @pnpm-syncpack/shared: 1.0.0 is developed in this repo at packages/shared/package.json
  workspace:* in dependencies of packages/api/package.json
  workspace:* in devDependencies of packages/app/package.json
  1.0.0 in version of packages/shared/package.json

Versions

Softwares

syncpack --version                                                                                                                                                                      12:59
8.2.4
pnpm --version                                                                                                                                                                          13:01
7.12.0

Machine informations

Model Name:	MacBook Pro
Model Identifier:	MacBookPro16,3
Processor Name:	Quad-Core Intel Core i5
Processor Speed:	1,4 GHz
Number of Processors:	1
Total Number of Cores:	4
Memory:	8 GB
System Firmware Version:	1731.140.2.0.0 (iBridge: 19.16.16066.0.0,0)
OS Loader Version:	540.120.3~19

Suggested Solution

If the version is specified in the local package and everywhere in the monorepo workspace:* is used as the version, just let pnpm handle this and don't raise error or make correction when running fix-mismatches.

Help Needed

Don't really now if this is a bug or if I missed an options or configurations to make pnpm workspace protocol and syncpack run smoothly together.

If this is indeed a feature request, I'll be glad to have some hints on where to start/search to implement it in a PR.

LudovicSterlin avatar Sep 20 '22 11:09 LudovicSterlin

Thanks @LudovicSterlin I'll take a look.

I'll know more when I look at this properly but see also #85 as it's loosely related, something in there may or may not help in the meantime.

JamieMason avatar Sep 20 '22 13:09 JamieMason

Awesome yes, setting workspace: false fixed it. Thank you Originally posted by @dhruv-004 in https://github.com/JamieMason/syncpack/issues/85#issuecomment-1176121423

Hey same for me here, I just added a .syncpackrc.yaml with just this line:

workspace: false

And no error any more. Thanks a lot 👍 🥳


However, to be a little picky I don't find very intuitive to set workspace: false while I'm still using locally developed package.

Also now, the comportment with this setting may cause conflict for people having mixed version and workspace protocol (not my case just thinking out loud, maybe not even a thing) since it let use any version for the shared package like 3.0.0 while the local available package is only 1.0.0 and it changes the workspace:* into 3.0.0.

I think a good thing could be to add in the doc the info that using workspace protocol is possible with workspace: false, may be with a warning on using only this and no mixed semver and workspace protocol.

Thanks for your reactivity. Please tell me if you want me to move what I discussed above into a feature request or a discussion 😉

LudovicSterlin avatar Sep 20 '22 13:09 LudovicSterlin

thanks @LudovicSterlin, agree something needs to be done. Please leave that repro up for a while and I'll use it to come up with some test scenarios – I don't use workspace: myself*, so that will be handy to understand how it's used.

* I just make sure the plain versions are in sync within the workspace.

JamieMason avatar Sep 20 '22 14:09 JamieMason

Yeah for sure, I just updated a bit the repo so if someone stumbles on it there is the right link for them to come back here.

LudovicSterlin avatar Sep 21 '22 07:09 LudovicSterlin

👋🏻 To anyone using "some-package": "workspace:*" versions – why do you use them? What do they offer vs using the same version number as the name property of that package in your workspace? I don't use this feature and with syncpack keeping the versions in sync, don't know of a reason why they'd be used.

Once I understand this functionality better, I'd be in a better place to know whether to do something to support it, or close this issue as not needed.

/cc @vith @aparajita (👍🏻'd this issue)

JamieMason avatar Feb 04 '23 09:02 JamieMason

The way I'm using versions in my monorepo, those internal deps are never on an older version of a monorepo package, I intend for them to be always current (the packages aren't published anywhere anyway, so the old version wouldn't be resolvable)

Also, when I'm bumping internal monorepo package versions I'm not using syncpack to do it; I'm using syncpack only while upgrading external dep versions

vith avatar Feb 04 '23 10:02 vith

workspace:* creates a link to an internal package and means whatever changes we make to the internal packages, regardless of their versions, are immediately available in their dependents. With anything but * after workspace: the dependent pins the link to a given semver range.

We need syncpack, on the other hand, to keep external packages consistent throughout a monorepo.

aparajita avatar Feb 04 '23 14:02 aparajita

Thanks a lot @aparajita @vith 👍🏻

JamieMason avatar Feb 04 '23 14:02 JamieMason

So in other words syncpack should ignore any workspace version specifiers, because those are handled by the package manager.

aparajita avatar Feb 04 '23 14:02 aparajita

Hi! @LudovicSterlin Does your repro still work? I had the same issue and tried to add: workspace:false and after running pnpm syncpack list-mismatches I'm still getting

@sample-package has mismatched versions which syncpack cannot fix      
  workspace:* in dependencies of apps\web\package.json

I even tried cloning your repo and got the same results :/

carlkentor avatar Feb 24 '23 07:02 carlkentor

Yes, indeed, I have the same problem - yarn 3, though, if that matters.

jeffrson avatar Feb 24 '23 17:02 jeffrson

Just updating this issue to correct outdated information.

workspace: false got removed as of version 9. The correct syntax to correct this going forward is

# .syncpackrc.yaml
dependencyTypes:
  - dev
  - overrides
  - peer
  - pnpmOverrides
  - prod
  - resolutions
  # - workspace <-- remove/comment this

Tested on yarn 3/4

joshmeads avatar Mar 26 '23 06:03 joshmeads

Yes, after I created such .syncpackrc.yaml, those messages are gone.

However, IMHO, "workspace: *" should be considered correct, if at least that other workspace exists.

jeffrson avatar Mar 29 '23 15:03 jeffrson

workspace:* is used by pnpm and abides by the spec for workspaces. https://pnpm.io/workspaces

The way it works is it will grab an internal version of the dependency in the workspace without having to specify a version. I just ran into this issue. In my case, I just used the version of the package in the package.json and I believe that fixed it. Going forward I will have to see if this matters or not. I don't think for packages built of HEAD we care about version alignment, hence the workspace:* syntax. But again, for us to link I think we need a version so maybe we just default to 0.0.1 for our internal packages and never use workspace:*.

Either way, would be nice if this worked, but not pressing.

Aghassi avatar Apr 21 '23 19:04 Aghassi

@JamieMason I was talking with my colleagues internally and we are curious how much work it is to support this change. In the past we used * for workspace packages, and having workspace:* would be more ergonomic for developers since we are building of HEAD and never versioning. Otherwise it's confusing to end devs since versions don't matter for us. Since syncpack already knows to check if something is pnpm, it feels like this should be part of that logic cascade that this is an acceptable version to find in that case. It's not ideal I understand, but ergonomically it meets the expectations of developers.

I can help with this fix if you can point me to the exact logic. I'll also look in my free time unless you beat me to it :)

Aghassi avatar Apr 24 '23 17:04 Aghassi

Any help would be great @Aghassi, thanks. First let's just sanity check that everyone here has a consensus on what the spec should be, to make sure we all have the same idea in our heads of what this would look like:

  • [ ] when a version mismatch is found
    • [ ] when any of the specified versions are of the format workspace:*
      • [ ] when someNewOption IS set
        • [ ] when the named package IS developed in this monorepo
          • [ ] then..? (Scenario A)
        • [ ] when the named package is NOT developed in this monorepo
          • [ ] then..? (Scenario B)
      • [ ] when someNewOption is NOT set
        • [ ] when the named package IS developed in this monorepo
          • [ ] then..? (Scenario C)
        • [ ] when the named package is NOT developed in this monorepo
          • [ ] then..? (Scenario D)

What we want to happen might vary between the scenarios A,B,C,D but possible resolutions I can think of are:

  1. Ignore them all?
  2. Set them all to workspace:*?
  3. Set them all to the highest valid semver version?
  4. Something else?

Questions

  1. I don't know if someNewOption is needed or not I just stuck that in, would one be needed for this? or would whatever we do be appropriate as the default behaviour, always?
  2. We wouldn't want to always just ignore mismatches whenever one of them is workspace:* right?
  3. Are there other scenarios I've missed?
  4. Anything to do with version groups that might matter?

If this is way off someone shout and feel free to correct me. Again, I don't use this feature and this is partly why I've taken a while on this, I'm not confident I know what's needed because I use exact version numbers and ensure they always match, so I've a blind spot to this whole thing.

Thanks all.

EDIT: I just had this idea which unfortunately does not work yet, pinned versions must be getting validated as needing to be valid semver:

{
  "versionGroups": [
    {
      "dependencyTypes": ["workspace"],
      "dependencies": ["**"],
      "packages": ["**"],
      "pinVersion": "workspace:*"
    }
  ]
}

JamieMason avatar Apr 24 '23 19:04 JamieMason

I would personally very much like something like an forcePnpmWorkspacePackagesWildcard (maybe less wordy) that makes sure all packages that are versioned workspace:<whatever> get's turned into workspace:*. All of them being workspace:* is sort of by definition without conflict. That being said - instead of using a boolean, it might be a better idea to use an enum kind of value: pnpmWorkspacePackage: force-wildcard, such that other modes might be supported?

Alxandr avatar Apr 25 '23 07:04 Alxandr

https://github.com/JamieMason/syncpack/issues/95#issuecomment-1520708328

I'll do my best to answer this given it's odd in our case because we are migrating from yarn 1.x to pnpm 7.x

  1. PNPM and Yarn have different resolving requirements. yarn requires that a workspace package have a valid semver to be resolved and linked. pnpm will only resolve and link if a semver version specified matches the semver version found in the monorepo. Otherwise, I believe it resolves (or attempts to resolve) from npm. workspace:* for pnpm is a special value that says "resolve any version in the workspace, it doesn't matter which one". In our case, this is always guaranteed to be HEAD of the monorepo, so we don't try beyond that to validate alignment.
  2. As for someOptionIsSet, I think what's more likely is you either want to detect there is an interop, or you want someone to be able to pass an object to workspace: true in the settings for pnpm. The object would look something like
"workspace": {
    "validSemvers": ["*", "workspace:*"]
    "pnpm": true,
    "yarn": true
}

or something like that and then rely on us to tell you which things we care about.

  1. I don't work in the world where we publish and consume internal dependencies back into a monorepo. That seems really odd to me. Though, I do understand there are folks who do it. I can't speak to this case, so I don't know what to recommend here.

  2. I think in general, the failure in the case of a monorepo should always fall towards recommending the user uses what's in the monorepo. I don't know the use case for consuming something published from a monorepo back into the monorepo. For the use case of something outside of the monorepo being consumed in, I feel like the current logic should just stand. I don't know how workspace:* works in that scenario though.

Is that helpful? I personally feel like if you find a pnpm-lock.yaml and nothing else, we should assume we are operating in a pnpm world and not need a config change. It's the case in which you find other lockfiles that you may need to ask for more info

Aghassi avatar Apr 25 '23 17:04 Aghassi

~To start with I've started to take a look at just having workspace:* be recognised as a supported value. As it is It's set as the 2nd highest version after *, so if any mismatching dependencies use workspace:* then the others will be synced to it. There's no checks at the moment to stop you from accidentally setting workspace:* on a package which isn't developed locally.~

EDIT: Since taken a different direction and treated workspace: as being a new kind of semver range.

As an aside on contributing, I love all the feature requests that have been accepted into syncpack and it's a lot better because of them, but the code is pretty complicated now. I've done two huge refactors already, mainly to support new features like custom groups etc which needed a whole different design, but also to try and manage maintainability. I think there's a lot of work to do there before it can be approachable to other contributors. I think a diagram of how it works at a high level would be useful too when I get some time.

One thing I find confusing from an end user POV is the workspace dependency type, because there's also this workspace:* thing out there in some of the package managers. Has anyone else found this awkward? Any ideas to clear that up a bit? Rename the dependency type to local or something?

There's no tests for these new changes yet either so this might take a little while.

JamieMason avatar Apr 28 '23 07:04 JamieMason

@JamieMason Happy to test any branches you make internally and let you know how it works. I appreciate all the work you do and understand the code is complex and hard to adjust at times.

Aghassi avatar Apr 28 '23 14:04 Aghassi

Thanks @Aghassi, I think I have something working on the dev branch if you could try that please.

I've tested it locally against https://github.com/microsoft/fluidframework with the following config as the first versionGroup, and it seems to be working:

{
  "versionGroups": [
    {
      "dependencyTypes": ["dev"],
      "dependencies": ["@fluidframework/**"],
      "packages": ["**"],
      "pinVersion": "workspace:*"
    }
  ]
}

JamieMason avatar Apr 29 '23 11:04 JamieMason

I gotta upgrade us to 9.0.0 first, then I'll report back :)

Aghassi avatar May 03 '23 17:05 Aghassi

Ok so this is an improvement. My edge case is this lol: I have both yarn and pnpm while we migrate. What doesn't seem to work (from my limited time spent on this) is when one package.json specifies the first party dep as workspace:* in prod or dev dependencies and another package.json specifies the package in peer deps with either an exact version or a * version. I know I can't put workspace:* in peer deps because yarn will fail to install since that's not a real semver. I think workspace:* only relates to dev and prod in theory. That being said, my other workspace:* declarations don't flag anything right now.

Here is an example of the error I get when I run your code (sanitized of names)

- @scope/package 3.1.2
  workspace:* in dependencies of root
  3.1.2 in peerDependencies of @scope/package-with-peer-dep

I'm not sure why it keeps checking peer even when I have the grouping set to prod and dev only

Aghassi avatar May 03 '23 18:05 Aghassi

Could it checking peer be the breaking changes in v9? https://github.com/JamieMason/syncpack/releases/tag/9.0.0

JamieMason avatar May 03 '23 22:05 JamieMason

@JamieMason I think I'm confused, what is breaking there? We already were checking peer in our prior state (or I thought we were)

Aghassi avatar May 08 '23 17:05 Aghassi

Can you open a separate issue to discuss this one @Aghassi? You might be fine but I thought, since you just updated to v9 and that the shape of the configuration file changed in that version, that that change could be the reason why your project is no longer checking peer dependencies in the same way as you expected. If you share your config file in that issue I can take a look.

JamieMason avatar May 08 '23 18:05 JamieMason

EDIT: Removed, I need to check a few things but this is closer to being done..

JamieMason avatar May 27 '23 21:05 JamieMason

Released in [email protected]. I will close this issue but please create a new one if you run into problems with a scenario which is not covered, thanks.

JamieMason avatar May 28 '23 11:05 JamieMason

Update: 12.0.0-alpha.0 adds better support for non-semver version formats and there is a new Getting Started guide.

JamieMason avatar Nov 08 '23 12:11 JamieMason