Skip to content

Introducing kiota fetch into Graph #712

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
nikithauc merged 16 commits into from
Mar 24, 2022
Merged

Introducing kiota fetch into Graph #712

nikithauc merged 16 commits into from
Mar 24, 2022

Conversation

@nikithauc
Copy link
Contributor

@nikithauc nikithauc commented Mar 18, 2022 •
edited
Loading

fixes #589

  • Introduces GraphBaseClient which will be extended by GraphServiceClient v1, beta and so on. The GraphBaseClient extends the client.init() and api() methods.
  • Removed RetryHandler, TelemetryHandler, RedirectHandler
  • Adding code to configure SDKVersions in the telemetry headers.

I have currently customized the GraphServiceClient as follows:

export class GraphServiceClient extends GraphBaseClient {
     public constructor(clientOptions: ClientOptions) {
        const c : GraphSDKConfig = {
            sdkTelemetryVersion: "graph-js-sdk-v1/0.0.0"
        }
        super(clientOptions, c);
        // change to this.authprovider
        const requestAdapter = new FetchRequestAdapter(clientOptions.authProvider, undefined, undefined, this.httpClient);        this.pathParameters = {};
 .
 .
 .
}

@nikithauc nikithauc marked this pull request as draft March 18, 2022 08:39
baywet
baywet reviewed Mar 18, 2022
View reviewed changes
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's huge progress! a few comments to help drive this further.

@baywet baywet mentioned this pull request Mar 18, 2022
Closed
@MIchaelMainer
Copy link
Contributor

MIchaelMainer commented Mar 18, 2022

Regarding your example in the PR description, let's maintain the header value according to spec:

    const c : GraphSDKConfig = {
       sdkTelemetryVersion: "graph-js-sdk-v1/0.0.0"
  }

It should look like graph-js-sdk/0.0.0 for v1.0

@sebastienlevert
Copy link
Contributor

sebastienlevert commented Mar 18, 2022

Regarding your example in the PR description, let's maintain the header value according to spec:

    const c : GraphSDKConfig = {
       sdkTelemetryVersion: "graph-js-sdk-v1/0.0.0"
  }

It should look like graph-js-sdk/0.0.0 for v1.0

I would actually argue that it should be

// Core
graph-js-core/4.0.0

// Service Library v1.0
graph-js/1.0.0

// Service Library beta
graph-js-beta/1.0.0

@MIchaelMainer
Copy link
Contributor

MIchaelMainer commented Mar 18, 2022

I'm going to argue against myself too and agree with @sebastienlevert

@nikithauc nikithauc marked this pull request as ready for review March 24, 2022 05:14
@nikithauc
Copy link
Contributor Author

nikithauc commented Mar 24, 2022

Related : microsoftgraph/msgraph-sdk-typescript#29

baywet
baywet approved these changes Mar 24, 2022
View reviewed changes
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for taking in the changes!

@nikithauc nikithauc merged commit 0111233 into 4.0.0 Mar 24, 2022
@baywet baywet deleted the nikithauc/v4-kiota-fetch branch November 30, 2022 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zengin zengin Awaiting requested review from zengin

@MIchaelMainer MIchaelMainer Awaiting requested review from MIchaelMainer

@rkodev rkodev Awaiting requested review from rkodev

1 more reviewer

@baywet baywet baywet approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nikithauc @MIchaelMainer @sebastienlevert @baywet