pnp / pnpjs

Fluent JavaScript API for SharePoint and Microsoft Graph REST APIs
https://pnp.github.io/pnpjs/
Other
740 stars 300 forks source link

feat(graph): add advanced query behaviour #3074

Open JakeStanger opened 2 weeks ago

JakeStanger commented 2 weeks ago

Category

What's in this Pull Request?

Hey, been a while!

Currently enabling advanced query capabilities is supported, but a two-step process:

const filter = "companyName ne null and NOT(companyName eq 'Microsoft')";
const query = graph.users.using(ConsistencyLevel()).filter(filter);
query.query.set("$count", "true");
const res = await query();

This PR adds a new behaviour which simply combines the two steps to make this more ergonomic:

const filter = "companyName ne null and NOT(companyName eq 'Microsoft')";
const res = await graph.users.using(AdvancedQuery()).filter(filter)();

I have updated docs but have NOT updated the relevant tests yet. If you can let me know whether you'd be happy to accept this change, I'll go through and update the tests.

patrick-rodgers commented 1 week ago

Hi Jake! Looks like good work to me :). The only thing I am not sure of is the name - would something like IncludeCount or WithCount or something else be more clear?

JakeStanger commented 1 week ago

Cheers, I'll try and update the tests for you in the next couple of days.

The only thing I am not sure of is the name - would something like IncludeCount or WithCount or something else be more clear?

My reasoning for the name is this performs the two 'setup actions' necessary for what MS refer to as 'advanced query capabilities'. I've just realised I helpfully left off the link to that in the original post: https://learn.microsoft.com/en-us/graph/aad-advanced-queries?tabs=http.

I'd say that naming it specifically after the count would not be correct, as it's doing more than that (you can enable count without the consistency level header).

If you disagree still, I'll change it and add a sentence or two explaining that the behaviour is useful for enabling advanced queries in the docs.

juliemturner commented 6 days ago

Yep, now that we understand your thoughts about the naming that makes sense. As far as testing we'd like to see a unit test specific to the feature, but wouldn't think we'd adjust existing tests, but I'm not 100% sure I have enough details so just wanted to clarify.

patrick-rodgers commented 4 days ago

Yep, thanks for explaining

JakeStanger commented 3 days ago

Cheers both. I've added what are effectively copies of the existing advanced query tests, using the new behaviour for these.