Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Comments

Close side panel

Introducing kiota fetch into Graph#712

Merged
nikithauc merged 16 commits into4.0.0microsoftgraph/msgraph-sdk-javascript:4.0.0from
nikithauc/v4-kiota-fetchmicrosoftgraph/msgraph-sdk-javascript:nikithauc/v4-kiota-fetchCopy head branch name to clipboard
Mar 24, 2022
Merged

Introducing kiota fetch into Graph#712
nikithauc merged 16 commits into4.0.0microsoftgraph/msgraph-sdk-javascript:4.0.0from
nikithauc/v4-kiota-fetchmicrosoftgraph/msgraph-sdk-javascript:nikithauc/v4-kiota-fetchCopy head branch name to clipboard

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.

src/MiddlewareFactory.ts Outdated Show resolved Hide resolved
src/MiddlewareFactory.ts Outdated Show resolved Hide resolved
src/authentication/SimpleAuthentication/SimpleAuthenticationProvider.ts Outdated Show resolved Hide resolved
src/requestBuilderUtils/GraphBaseClient.ts Outdated Show resolved Hide resolved
@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.

4 participants

@nikithauc @MIchaelMainer @sebastienlevert @baywet
Morty Proxy This is a proxified and sanitized view of the page, visit original site.