oidc-signin-tool has a direct dependency on a package from this monorepo, it should be a peerdependency

[simnorm]

See package.json

Notice that it has a direct dependency on:
"@itwin/service-authorization": "workspace:^",

This should be a peepDependency ^2.1.0 and a devDependency of "workspace:^"

Otherwise, it forces all consuming apps to be on the same version of @itwin/service-authorization and leads to doppelgangers with other iTwin or Bentley packages.

I believe that are plenty examples of this in other iTwin package.json

[saskliutas]

I am curious does this cause any real issues? Initially I assumed that @itwin/service-authorization has statics and those cause issues if multiple versions are resolved in node_modules. However, I looked over @itwin/service-authorization and it does not seem like there are any statics used. There should be no issues if multiple versions are resolved.

One possible issue I see is that @itwin/service-authorization has dev dependency on @itwin/core-common and exposes type from it

auth-clients/packages/service/src/ServiceAuthorizationClient.ts

Line 27 in a7a2885

export class ServiceAuthorizationClient implements AuthorizationClient {

I believe this might cause issues if multiple versions @itwin/core-common are resolved. I think that should be a peer dep and @itwin/oidc-signin-tool should have it as peer dep as well if it keeps direct dep on @itwin/service-authorization.

cc @aruniverse @ben-polinsky

[ben-polinsky]

I am curious does this cause any real issues?

I agree with this assessment. If we want to make that change we can.
@simnorm are you seeing a specific issue where this is causing problems?

I believe this might cause issues if multiple versions @itwin/core-common are resolved.

This does seem like an issue.

Thanks @saskliutas

For now, I will draft my PR.

[saskliutas]

I believe this might cause issues if multiple versions @itwin/core-common are resolved.

This does seem like an issue.

On the second thought this probably shouldn't cause issues for consumers as that's a dev dep and it won't be installed for consumers. So there should not be multiple core-common versions resolved.

I haven't experimented but there are possible issues with types if dependencies hoisting is disabled. Not sure how typescript would handle that as core-common should be invisible to service-authorization without hoisting.

[hl662]

@simnorm — the fix is straightforward and the change itself is correct convention. However, moving
@itwin/service-authorization from dependencies to peerDependencies is a breaking change — consumers that relied on it
transitively would now need to add it explicitly. That warrants a major version bump to @itwin/oidc-signin-tool and an ask
for consumers to update.

Before we go that route, we want to make sure the issue warrants it. @saskliutas reviewed @itwin/service-authorization and
didn't find statics or other doppelganger-sensitive patterns, so there's no confirmed runtime breakage today.

Can you confirm: are you seeing a concrete problem in your app? Thinking, version conflicts, unexpected behavior, resolution
failures? If so, it would help us validate that a major bump is the right call here.

[simnorm]

@hl662 I don't believe we had problems directly with this package being a doppelganger. The problem was more with other packages that would consume it. @itwin/oidc-signin-tool and @bentley/civil-backend.
A few months ago, this had already forced us to not rely on it being a transitive dependency and explicitly specify a version.

Recent update to @itwin/oidc-signin-tool moved to using 2.1.0, while Civil stayed on ^2.0.0.
Our app started getting 2 different civil-backend packages which I believe has statics.

So bottomline is that any app that consumes 2 packages of that require @itwin/service-authorization will likely need to tinker with versions... I'm really not sure what is the best way forward if you fix this and make the breaking change....

[hl662]

Okay...
I'm in favor of adding the breaking change. We can break this now so our consumers won't deal with this problem long term.
We have a PR that upon merging, will lead to v6.0.0 of oidc-signin-tool.

Consumers who only use oidc-signin-tool for testing and no service-authorization usage in their source code will need to add @itwin/service-authorization explicitly to their devDependencies: "@itwin/service-authorization": "^2.1.0"

While consumers who do use service authorization in source code (and use oidc-signin-tool as a test tool) would already have service-authorization in their dependencies section of package.json, so that shouldn't affect them.

We can communicate explicitly about the impact through GH releases and the changelog being added, as well as going through our internal communications channel to notify our internal first party users.

@ben-polinsky @saskliutas Any extra comments?

[aruniverse]

Recent update to @itwin/oidc-signin-tool moved to using 2.1.0, while Civil stayed on ^2.0.0.
Our app started getting 2 different civil-backend packages which I believe has statics.

Are we sure this is due to us incorrectly configuring our dependency or this is an issue with the civil packages?

[hl662]

It's on us, I checked civil, and OS+.

@itwin/oidc-signin-tool had @itwin/service-authorization as a direct dependency when it should have been a peer. That forced a second copy into the tree, and civil-backends statics made the conflict visible.

civil-backend is configured correctly, they set service-authorization to be peerDependencies: "^2.0.0".

v6.0.0 of @itwin/oidc-signin-tool fixes this by moving @itwin/service-authorization to a peer dependency, which doesn't change civil packages, it's going to fix the problem one level downstream further - where we have apps that use service-authorization in app, and also oidc-signin-tool as a test tool. Our other next gen+ apps should run the risk of this problem too no?

[saskliutas]

Recent update to @itwin/oidc-signin-tool moved to using 2.1.0, while Civil stayed on ^2.0.0.
Our app started getting 2 different civil-backend packages which I believe has statics.

It would be interesting to look into why 2 different civil-backend were resolved. From what I understand package.json should look something like:

"dependencies": {
  "@bentley/civil-backend": "x.x.x",
  "@itwin/service-authorization": "2.0.0",
}
"devDependencies": {
  "@itwin/oidc-signin-tool": "5.0.0",
}

Looking at this I would expect package manager to resolve civil-backend to use @itwin/service-authorization@2.0.0 while @itwin/oidc-signin-tool could be resolved with different copy @itwin/service-authorization@2.1.0.

@itwin/oidc-signin-tool had @itwin/service-authorization as a direct dependency when it should have been a peer. That forced a second copy into the tree, and civil-backends statics made the conflict visible.

I don't think having second copy of @itwin/service-authorization is inherently bad. There is no requirement to have only single instance of this package in the app. unlike other @itwin packages with statics.

civil-backend is configured correctly, they set service-authorization to be peerDependencies: "^2.0.0".

I don't thins that civil-backend should have peer dep on service-authorization. Peer deps should be set on packages that expose statics and have requirement for single package copy in node_modules of the app.

v6.0.0 of @itwin/oidc-signin-tool fixes this by moving @itwin/service-authorization to a peer dependency, which doesn't change civil packages, it's going to fix the problem one level downstream further - where we have apps that use service-authorization in app, and also oidc-signin-tool as a test tool. Our other next gen+ apps should run the risk of this problem too no?

I guess this is a viable solution but now we are adding new peer deps just to band-aid a solution for some other problem (why 2 civil-backend version were resolved? problem in package manager? problem with package manager config or civil-backend deps?).