search

SharePoint / PnP-JS-Core

Code moved to https://github.com/pnp/pnpjs. This repository is archived.
Other
379 stars 231 forks source link

Add support for `expose` in `getAll` #746

Closed eirikb closed 6 years ago

eirikb commented 6 years ago
Q A
Feature enhancement [ x ]

What's in this Pull Request?

This commits add support for expose to the new getAll method. We discussed this on gitter, and I understand that expose was deliberately excluded from getAll because of performance reasons. While I understand the decision I still would like the ability to get all data in the exact same was as with items.get(). My fix will re-use the original query, so it should "just work", even if the original query would change layout somehow (unlikely).

The reason I want this is that we often use pnp for small snippets (using tick), and then we need to run expose to do one-time jobs where we need more data, even if they are slow.

P.S. I was originally suppose to just make an issue, but wanted to try making a PR, so this is my first to this repo. I really like pnp, and making PRs would be a nice way to help out.

patrick-rodgers commented 6 years ago

Hi @eirikb - thanks for this, assume you mean "expand". Issue is that you have rewritten the implementation so that it no longer behaves how it was prior to your change. For example removing the nometadata configuration and the filter for the allowed odata operations. Any particular reason you chose to drop those features?

Edit: In testing this change also introduces behaviors when used with batching and caching that are inconsistent. Dropping all these sorts of things was another reason I had created a new Items instance. For example the below code returns the accurate number of items for h, but only the first set for h2 (2000 by default).

pnp.sp.web.lists.getByTitle("BigList").items.usingCaching().getAll().then(h => {

console.log(h.length);

    pnp.sp.web.lists.getByTitle("BigList").items.usingCaching().getAll().then(h2 => {

        console.log(h2.length);
    });
});
eirikb commented 6 years ago

@patrick-rodgers Hi, sorry yes I meant $expand. And sorry, I made a mess of the code, I didn't intend to remove any functionality, I honestly thought that I could re-use the local query directly, but I totally forgot about the caching mechanism. Changed the PR now to only add the $expand to the query, as that was my original intention.

The reason I removed configure was because re-configuring won't work with adal, or probably any other custom config, as it would remove headers.
Should probably re-write to Object.assign headers, and then check if users have not specified a specific Accept header.

patrick-rodgers commented 6 years ago

All good, I did like what you did with returning the promise chain directly and not newing up a new Promise object. That is a nice change to work in and makes the code a little cleaner.

Talk to me more about the configure thing - is it breaking something? We do copy the options object from the "parent" object to the new in the contructor - are we missing something?

eirikb commented 6 years ago

I might be mistaken about the configure thing, I just noticed I got permission denied when running in node (with adal). Just did a test, and it seems getPaged also fails. Then I added some logging, and it seems it's the second request of getPaged which fails, could it be that this line: https://github.com/SharePoint/PnP-JS-Core/blob/master/src/sharepoint/items.ts#L390 doesn't get all the configuration copied over?

patrick-rodgers commented 6 years ago

Can you create an issue for that and include your code? Does sound like a bug so want to make sure we track it as such. We are going to make using ADAL easier, but that work will likely be in the new libraries.

koltyakov commented 6 years ago

@eirikb I would also suggest moving to @pnp/* in general. We migrated a couple of our internal projects to the new one and all went well and smooth.

eirikb commented 6 years ago

@patrick-rodgers Made an issue on pnp now: https://github.com/pnp/pnp/issues/31 Seems to be a problem there as well. What happens to this PR now? The 403 isn't really related, and I only want the $expand to work. Should I also add this PR to pnp/pnp?

patrick-rodgers commented 6 years ago

You can add it both places, or just leave it here and I'll move it over - which is what I have been doing. BUT if you do add it to pnp/sp I can add you to the authors file over there too :)