Pnpjs: [proposal] Do not rely on fetchClientFactory configuration in Node.js

[michal-kocarek]

Category

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

Desired Behavior

We are experiencing same issue, as in #589 in our company. We need to connect from single Node.js process to multiple SharePoint sites, and what is even worse, we would appreciate to connect to one site using two different credentials.

Proposal

📝 I started to take a look in this issue and have WIP code that would allow this functionality:
https://github.com/pnp/pnpjs/compare/version-2...michal-kocarek:version-2

It would be great that each instance of SPRest could optionally have own fetchClient, which would be used in all subsequent requests initiated from this instance, including batch requests. To maintain compatibility, fetchClientFactory would be used as a fallback, but it would be optional.

const sp1 = sp.configure({fetchClient: new SPFetchClient(url1, clientId1, clientSecret1)});
const sp2 = sp.configure({fetchClient: new SPFetchClient(url2, clientId2, clientSecret2)});
// also possible
const sp3 = new SPRest({fetchClient: new SPFetchClient(url3, clientId3, clientSecret3)});

// to support batch processing, baseUrl must be provided to SPRest configuration:
const spWithBatchSupport = sp.configure({fetchClient: new SPFetchClient(url, clientId, clientSecret)}, url);
const spWithBatchSupport = new SPRest({fetchClient: new SPFetchClient(url, clientId, clientSecret)}, url);

Plan:

• ✅ standard SP queries (done)
• ✅ batch queries (done)
• tests
• documentation

Open questions:

• is this even right direction? To me, being able to pass own fetchClient to SPRest makes sense from good design point of view... I would appreciate some thoughts if this could be taken even further or whether it's absolutely wrong direction :)
• how will this affect caching (technically, cache should be specific of each client+baseUrl I guess... but we can perhaps skup it for now?)
• ability to remove _spPageContextInfo.webAbsoluteUrl hack from SpFetchClient - it seems that it should be doable, but I need some time to look fix the batch class first
• graph libraries - should they be also updated

Thank you for your help and feedback...

[koltyakov]

Looks interesting @michal-kocarek.
I did slightly bit different approach while keeping the existing configuration layer.
The approach was to use a multi fetch client with detection which client to use based on a custom header with instance ID and passing that header via configure method.

[michal-kocarek]

Thanks, @koltyakov. We have been using same method first in our project, but then it caused some nasty issues, as the headers were not passed all the time.
There is for example the digest retrieval logic inside SPHttpClient (https://github.com/michal-kocarek/pnpjs/blob/version-2/packages/sp/sphttpclient.ts#L231-L237), that is completely skipping passing of custom headers. So here it won't work.
Another issue might be the SPBatch, which creates the SPHttpClient on it's own and I am not sure if it is passing the headers through...

I know that preferred way to use the library is to use global instance of sp, but it is essentially just instance of SPRest class, that sometimes uses global stuff (like fetchClientFactory or various cache layers across the modules, to leverage use in various cases, like running in context of client-side SharePoint app, which has all the context already available). So the cleaner approach to me would be to just allow SPRest class be created with fetch client, and then let it naturally pass through. Obviously this would need to be optional to not break existing patterns...

[michal-kocarek]

Support for batch request has been added. To be able to use it without fetchClientFactory, baseUrl must be passed to the SPRest instance as second argument.

This is overcoming the fact that in SPFetchClent, global variable _spPageContextInfo.webAbsoluteUrl is set to the base URL of the site (which conflicts when two SPFetchClients are present). However it seems to be used only in few cases in toAbsoluteUrl(), and right now, when baseUrl is passed to the SPRest, both cases (in operations.ts and in batch.ts which call toAbsoluteUrl() will never use that global variable). What is your view on this, @patrick-rodgers, does it make sense to keep this hack and/or to remove it? Obviously... If even some part of this PR makes sense to you :)

[patrick-rodgers]

@michal-kocarek - I am still on family leave but this is super interesting. Once I am back next week I will have a look as soon as I can direct some energy towards PnPjs. Appreciate the work and ideas, excited to look deeper and work with you on figuring out what works best. Thanks!!

[michal-kocarek]

@patrick-rodgers Sure, let me know when you'll be back, I am interested in your thoughts, as when we settle on some agreement, I'd like to add tests to finalize this PR... Have a nice day.

[patrick-rodgers]

Hi @michal-kocarek - been talking through this with @juliemturner and we understand the need and idea, which we agree are good. The problem is now that we've had a chance to really think through this the changes end up being pretty large because they cascade into so many areas. Each instance would need to carry knowledge of it's specific configuration which today is global. This also affects all the places we clone objects or create new instance etc. - so we would be touching lots of code. Which is OK!

It does mean we need to take a bit more time and really think thought how we do this - because it will affect the entire library. We also must be mindful of not breaking all of the current users to add this capability. So we agree this is a good change and something we want to support, but will really come down to moving to an entire new library architecture (and likely a minor release to call it out). We also would then need to make the same changes for the graph library.

What does that mean for a timeline? Good question. It will not make it into the July release. Maybe August, but I can't commit to that. Right now we (Julie and I) agree this can target for September.

Sound good as a plan forward?

[michal-kocarek]

Hi. Thanks for a quick discussion... I fully understand your reasoning. It makes sense, and the timeline sounds okay as well.

Is there some way how can I support the effort? E.g. if we agree on incorporating the steps in small pieces, I can provide PRs for those small pieces and also add some level of tests that would cover client-side + server-side cases, so we could make sure it won't break anything...

[juliemturner]

@michal-kocarek -- We can let you know but our first goal is to get the architecture right for the change as it's fairly significant as @patrick-rodgers indicated. If we didn't care about backward compatibility, it's fairly straight forward, but because we do and we want to maintain a simple implementation for those that don't need this feature, which is arguably most, we need to give it some amount of attention before deciding how to implement it. I agreed with Patrick it's a great idea, just more far-reaching than it might seem on the surface.

[michal-kocarek]

I see... If there would be anything I can be helpful with, even as partial preparation to get this done, just let me know. Otherwise I appreciate your willingness to incorporate this change in future...

[patrick-rodgers]

Thanks @michal-kocarek - let us knock it around a bit more and let's see if its appropriate to split out some of the work, very much appreciate your offer to help :). For sure help with testing a beta version will be great, but let's see if we can figure out a way to get you coding!

[patrick-rodgers]

Have found the time to add the ability to create isolated library instances as discussed in this issue. These changes are not yet merged, but a beta is available npm install @pnp/sp@beta for testing. Ask that you please give it a try and let us know what you think of our first pass at enabling this functionality, as well of course of any issues you encounter.

The initial docs are available in the PR and outline how to use the new capabilities.

Very interested in any feedback you have. This is a large change so we will be doing additional testing and may update any of the public methods in the beta as we work with it.