Pnpjs: sp-clientsvc should be a dependency of sp-taxonomy and not a peerdep

[baywet]

Category

• [ ] Enhancement
• [x] Bug
• [x] Question
• [ ] Documentation gap/issue

Version

Please specify what version of the library you are using: [1.1.2]

Expected / Desired Behavior / Question

When I install @pnp/sp-taxonomy, I expect @pnp/sp-clientsvc to be installed as well. The reasoning is the following: sp-taxonomy won't run if sp-clientsvc is not available at runtime. Hence the taxonomy package DEPENDS on it.

Observed Behavior

Today most of the packages list one another, including the hard dependencies, in peerDeps. which doesn't pull automatically the targeted packages during installation.
Before changing anything, I'm curious to know what is the reasoning behind that choice?

Steps to Reproduce

npm i @pnp/sp-taxonomy -S

(write some code relying on sp-taxonomy and try to build it)

Error: Can't resolve '@pnp/sp-clientsvc' in 'C:\sources\gitttl\clientname\IntranetPortal\node_modules\@pnp\sp-taxonomy\dist'

[patrick-rodgers]

The reasoning is that all the packages depend on each other as peers. So that version x.y.z of @pnp/js always depends on x.y.z of @pnp/common etc. This avoids us having to do any regression testing or maintain a complex version graph of what works with what. They are peers in that they should all exist at the same "level". We want to also avoid the scenario where say I install x.y.z of all the libraries and now I upgrade just @pnp/sp. If it had everything listed as dependencies it would pull those in under itself. Now in my project I at a later time upgrade the graph package, so it will pull in its new dependencies - and when I bundle everything I end up essentially doubling most of the code in the package. Now start doing that for each of the packages such as common, odata, graph, etc where they all end up with their own dependency tree. It is my belief that these packages are peers and this correctly expresses the relationship.

I think what this issue is really asking is "why do I have to install multiple things". If that is a big blocker you can use the @pnp/pnpjs package that is a rollup of the core packages and does list them as dependencies.

To address the specific case of the taxonomy library, perhaps it could be a dependency - but it was left this way to match the other packages and to look ahead in case we have additional things that want to share the clientsvc in the same way as common for example.

[baywet]

Hi Patrick, thanks for the detailed reply.
The newer versions of npm avoid nesting of packages by default if they don't need too, mostly because of windows path length and duplication of modules.
I think the trick to avoid duplication of packages by npm (and later on by the bundler) is not to have too restrictive versions requirements. Eg let's assume @pnp/sp and @sp/sp-taxonomy depend both on @sp/odata. If you fix the version every time (1.1.2 now, 1.1.3 tomorrow) and if we take your upgrade scenario, yes npm is going to store 2 versions of @pnp/odata, one under the sp one, one under the taxonomy one.
However if the version definition is a bit more flexible (let's say ^1.1.2 for eg and ^1.1.3 tomorrow), npm should be able to resolve the "conflict", take the highest matching version and have this one at the root node_modules folder.
So I can understand if you want to leave it as is for simplicity of maintenance, however I'm still advocating for deps instead of peer deps for simplicity of usage.

[patrick-rodgers]

The issue with that approach is that you are then asking us to test all the packages against all the versions to ensure stuff works, and to add the complexity to the build process to ensure we are putting in the correct version mask. It also means that when someone reports a bug we have to determine exactly what version of each package they have, get setup that way, test it, etc. MUCH MUCH easier to keep everything in lockstep. And again, if installing multiple packages is a blocker you can always fall back to @pnp/pnpjs. We have to make at least a few decisions to help us more easily maintain things. :)

[patrick-rodgers]

Going to close this as by design / answered.

[baywet]

yep! thanks for the explanation, now I understand how much of an additional burden it'd be for you guys!

[baywet]

@patrick-rodgers follow up on our discussion and additional argument for the deps https://github.com/SharePoint/sp-dev-docs/issues/2207#issuecomment-413727681 :)

[patrick-rodgers]

I wasn't aware of the need to specify the dependencies, we'll update our docs. Still doesn't address all the other issue with using dependencies vs peer dependencies.

[baywet]

yeah I know it wouldn't make maintenance any easier on the pnp side of things. It'd only make CDN external settings (listing another pro, that's it). But thanks for the follow up!

[patrick-rodgers]

I am looking into this deeper. We do list our dependencies, but as peer deps, which the packaging for spfx ignores when it produces the manifest files. I am not sure that is right or wrong, but it does make it a bit hacky to use things from the cdn as is. So we are again faced with the question of taking on more work to support making things easier based on another libraries choices.

[baywet]

Understand the dilemma. I'd be curious to know whether that external behavior is a Webpack choice or a SPFX choice at this point? I'd have to try with a plain webpack project and not a SPFX project...

[patrick-rodgers]

Looking at the spfx source code the issue is they are ignoring peer deps when they write the manifests they use for packaging. I found the line and the resolution would be adding a single line, so I am trying to see if that can happen. Ignoring pnpjs I think it is the right thing to do as any package can have peer deps that should be processed like deps.

[baywet]

As they have a hard dependency on webpack, I'd expect the behavior to be fairly similar as webpack unless they have a very good reason not to do it. But thanks for looking into the SPFX source!