Add support for using nextLink in the typescript sdk

[waldekmastykarz]

In the TypeScript SDK, with a GET request, you get a nextLink, but there doesn't seem to be a way to use it, other than passing it manually to fetch outside of the Graph SDK. The SDK should have a native support for retrieving data using the nextLink.
AB#10398

[baywet]

welcome to the repo @waldekmastykarz !! 👋

This is very valuable feedback. Right now the way of doing that would be the following:

const page1 = await apiClient.usersById('').messages.get();
const page2 = await (new MessagesRequestBuilder(page1.nextLink, apiClient)).get();

But it's not going to work at the request builder will try to append the path segment to the URL and might also mess up the query string parameters.
We probably need to add another boolean parameter to the request builder constructors appendPathSegment defaulted to false. In the request builders that instantiate the children, we'd pass true but a consumer would simply need to keep only the default and it'd work.
The decision between appending the path segment and setting the URL property of the request info probably needs to move inside of the abstractions library as well to avoid generating too much code.

[waldekmastykarz]

It seems pretty complicated to have to do it that way. Since nextLink is a complete URL, what if we let developers to do something like apiClient.get<MessagesResponse>(page1.nextLink)? If we'd prefer not to expose a generic get/post on the ApiClient, we could also consider page1.nextPage() (since nextLink is always a GET, it's shorter than page1.nextLink.get()). What do you think?

[baywet]

there are a couple of different components to that approach:

• most graph core libraries already allow you to do something like that, and maybe part of the alignment work is to make sure this is still possible
• graph core libraries also have a page iterator task, walking all the pagination for you and calling a callback on new items
• odata next link is really an odata concept, and we don't want to have kiota being responsible for odata concepts
• exposing a generic get/post/patch/put on the client (kiota only) would defeat the purpose of a fluent API generator
• plus it'd be missing things like headers, middleware options etc (arguably, we could add these additional arguments)
• you can technically already do what you are proposing using the http core (httpCore.sendAsync(theRequestInfo, MessagesResponse)
• I think wanting to use existing request builders while providing the full link is a valid use case (there are plenty of cases where you would save/transfer a URL for later use, think about bookmarking, push notifications, offline access...) and having this ability with the request builders makes a ton of sense, hence my original suggestion.
• now if we have better suggestions that do not make OData assumptions, I'm happy to discuss those.

[waldekmastykarz]

odata next link is really an odata concept, and we don't want to have kiota being responsible for odata concepts

I understand that. At the same time we need to think about how developers will use our SDK. Ideally, we should strive to make things as easy as possible for developers, even if it means that our responsibility expands to generic concepts beyond our services. I don't think that developers look at our API the same way we do and split it between Graph and OData concepts. They want to get the data they need with as little work as possible and move on to the next task. Getting out of their way and making our SDKs as simple to use as possible is key to adoption and developer satisfaction.

exposing a generic get/post/patch/put on the client (kiota only) would defeat the purpose of a fluent API generator

If the SDK gives me back a URL, I expect to be able to use that URL with the SDK. If we offer our developers only the internals requiring them to build request info manually, then I'm pretty sure that folks will come with a helper method allowing them to pass the URL and get back the the request info object. And it would be a shame for everyone to have to implement such a helper themselves if we can solve it for everyone in the SDK.

graph core libraries also have a page iterator task, walking all the pagination for you and calling a callback on new items

The iterator is helpful if you want to retrieve all items in the collection (eg. all groups or all channels in a Team). But if you just want to retrieve next batch of items (eg. next set of emails, where you'd almost never want to get all emails at once), it's overly complicated and being able to get just the next batch with a single line of code would be more user-friendly.

I appreciate discussing the different perspectives together. I'm sure we'll find a way to solves this that works for everyone.

[baywet]

Those are all valid points. To extend on the OData aspect I'd say that in my experience most graph users don't care/know enough about OData to consider its ramifications. It's usually a more transactional approach "I send you this request, give me the result".

Would this be acceptable, or is it still too complicated? (should already be possible today)

const page1 = await apiClient.usersById('').messages.get();
const page2Request = {URL: page1.nextLink, httpMethod: HttpMethod.GET } as RequestInfo;
const page2 = await httpCore.sendAsync(page2Request, MessagesResponse);

[waldekmastykarz]

I think it's odd to have to use two different entrypoints to execute a request (apiClient and httpCore) and might end up confusing developers when to use which. What if we could attach sendAsync or even a shorthand get to apiClient so that it's a single entrypoint for all Graph operations?

[baywet]

so it'd give us something like that:

const page1 = await apiClient.usersById('').messages.get();
const page2 = await apiClient.get(page1.nextLink, MessagesResponse);

What do you think?

(we'd probably have another sendAsync method as well for anything more complex than a get)

[waldekmastykarz]

I like it a lot! What is the MessagesResponse argument for?

As for the naming, isn't the Async suffix a .net-ism? I'm not sure I've seen it a lot in the JS world. Without Async, we'd also align .messages.get() and apiClient.get() 💪

[baywet]

Yep sorry, I was writing CSharp at the same time I was responding... Edited my response.
The MessageResponse argument is to know what type to deserialize to, which constructor to use.

[waldekmastykarz]

The MessageResponse argument is to know what type to deserialize to, which constructor to use.

What if we'd pass it as a generic? Wouldn't that be more intuitive?

const page1 = await apiClient.usersById('').messages.get();
const page2 = await apiClient.get<MessagesResponse>(page1.nextLink);

[baywet]

sure but I still need the factory for the type. Typescript doesn't have generic type constraints like : new() in dotnet. So I won't be able to new T() below during deserialization. And reflection is not really a thing, so I can't do something like typeof(T).getConstructor().newInstance(); unless I'm missing something?

[waldekmastykarz]

If you deserialize, wouldn't it work to do:

return JSON.parse(stringResponse) as TResponse?

Or do you need to know the type upfront because each type has its own deserializer. Then you could use {} as TResponse, eg.

abstract class ResponseMessage {
    public abstract deserialize(stringResponse: string): void;
}

class MailResponse extends ResponseMessage {
    public deserialize(stringResponse: string): void {
        const o = JSON.parse(stringResponse);
        // set properties on the current instance
    }
}

function get<TResponse extends ResponseMessage>(url: string): TResponse {
    const r: TResponse = {} as TResponse;
    const stringResponse: string = '';
    r.deserialize(stringResponse);
    return r;
}

const mail = get<MailResponse>('/me/messages');

[baywet]

If you have a look at our deserialization process, you'll noticed we don't rely directly on JSON.parse, but instead we've introduced an abstraction layer. This has multiple advantages:

• default values are set
• methods are available
• we can support different serialization formats with the same generation output (XML, YAML, gRPC...)

To do so, JsonParseNode instantiates the target type, so it needs to know about it.

[baywet]

FYI I started working on that with #508

here is what it'll look like

const page1 = await client.me.messages.get();
const page2 = await new MessagesRequestBuilder(page1.nextLink, core).get();

And of course we'll be able to add a page iterator in the graph core library like we have today.
Thoughts?

an alternative would be to add a helper method on the core service and do something like

const page1 = await client.me.messages.get();
const page2 = await core.get<MessageCollectionResponse>(page1.nextLink);

I'm not sure which one is more intuitive granted .messages gives you a MessagesRequestBuilder (easy to find and look at the constructor) and .get gives you MessagesCollectionResponse, but the alternative will be harder to discover granted one would have to thing about looking at the http core service, understand the generic aspect etc...

[waldekmastykarz]

I'm still not quite convinced about the requirement to use both client and core. I get why we need it, but I suspect it will end up confusing folks and they will end up creating a wrapper that abstracts it away.

Is there a way to attach .get<T>() to client, so that devs can call client.get<T>(...)? If I recall correctly, you mentioned that you'd prefer to decouple the implementation of both classes, but what if internally client would pass the call to core and all that client.get() is, is an alias for core.get() in favor of consistency for SDK consumers?

[baywet]

One of the goals of Kiota was to keep all the fluent API surface (including the API client, which is really a request builder with a special constructor) super lean so the generation logic stays simple and so we can handcraft great APIs at the "manually written code".

I do think wanting to "keep everything on the client" is a bias due to the existing SDKs. Sure it'd make for a shorter code path but that's about it.
Besides proxying the method from core to the client (adds a lot of complexity to the generation process), we could provide an accessor to the http core on the client, so people don't have to keep tracks of two things, in which case it'd look something like:

const page1 = await client.me.messages.get();
const page2 = await client.core.get<MessageCollectionResponse>(page1.nextLink);

It'd be good if @roinochieng could provide input on this as the PM and the experience we want to deliver.
As a reminder the alternatives are

const page1 = await client.me.messages.get();
const page2 = await new MessagesRequestBuilder(page1.nextLink, core).get();
// could be client.core instead of just core here

const page1 = await client.me.messages.get();
const page2 = await client.get<MessagesResponse>(page1.nextLink);

[waldekmastykarz]

Sure it'd make for a shorter code path but that's about it.

I think it's also about avoiding unnecessary complexity and exposing internals to our consumers, which are important parts of an SDK.

Let's get @roinochieng's input indeed 👍

[nikithauc]

I would like to jump in here :)

Of the two alternatives, the following seems intuitive.

const page1 = await client.me.messages.get();
const page2 = await client.get<MessagesResponse>(page1.nextLink);

1. In case of any request, for example, client.users.get() the request starts with the client and the above alternative keeps that format intact.

2. As @waldekmastykarz mentioned, the users can get confused and it requires the user to know about the core implementation all the time giving away the benefit of an abstraction.

@baywet I believe the point that @waldekmastykarz is making about having direct calling functions in the client should be included in the GraphServiceClient function and I can think of an easy manaual way to have it implemented in the GraphServiceLibrary.

[baywet]

Thanks @nikithauc, I think having those sorts of method in the GraphServiceClient library in the service library is a good compromise.

The implementation would stay simple and should look roughly something like

public get<ResponseType extends Parsable>(url: string) : Promise<ResponseType> {
     const requestInfo = new RequestInfo();
     requestInfo.method = HttpMethod.GET;
     requestInfo.setUri(url, undefined, true);
     return this.httpCore.sendAsync(requestInfo);
}

I'm going to go ahead and create a separate issue to track this additional method (which we can eventually transfer to the service lib repo once we have one?) as this one will get closed once the pull request gets merged.