Skip to content
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.

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

feature/request builders proxy #3938

Merged
merged 40 commits into from
Jan 23, 2024
Merged

feature/request builders proxy #3938

merged 40 commits into from
Jan 23, 2024

Conversation

baywet
Copy link
Member

@baywet baywet commented Dec 19, 2023

fixes #3642

@baywet baywet self-assigned this Dec 19, 2023
@baywet baywet force-pushed the feature/request-builders-proxy branch from 5cc4b5a to 214a0b4 Compare December 20, 2023 17:37
@baywet baywet marked this pull request as ready for review December 20, 2023 19:09
@baywet baywet requested a review from a team as a code owner December 20, 2023 19:09
@baywet
Copy link
Member Author

baywet commented Dec 20, 2023

@andrueastman another big PR for you to review if you have some available time.
Note: the changelog entry is missing

@baywet baywet force-pushed the feature/request-builders-proxy branch from 37c1ef0 to e7e8304 Compare January 10, 2024 19:06
@sebastienlevert
Copy link
Contributor

I forgot to paste my feedback on this PR before the holidays:

  • newXClient should become createXClient
  • We're still generating an entire tree so no available tree shaking. Something to look into.

Webpack stats with kiota 1.9.1 vs. this PR:

// kiota 1.9.1 + npm modules
asset bundle.prod.js 1.14 MiB [emitted] [minimized] (name: main) 2 related assets
asset 29770313afeede1010fc2402cc7cc6d0.node 75.2 KiB [emitted] (auxiliary name: main)
orphan modules 1.57 MiB [orphan] 435 modules
runtime modules 1.06 KiB 6 modules
built modules 2.09 MiB [built]
  modules by path ./node_modules/ 1.2 MiB 153 modules
  + 19 modules
webpack 5.89.0 compiled successfully in 7537 ms

// kiota local + npm via link
asset bundle.prod.js 1.14 MiB [emitted] [minimized] (name: main) 2 related assets
asset 29770313afeede1010fc2402cc7cc6d0.node 75.2 KiB [emitted] (auxiliary name: main)
orphan modules 1.59 MiB [orphan] 437 modules
runtime modules 1.06 KiB 6 modules
built modules 2.11 MiB [built]
  cacheable modules 2.11 MiB
    modules by path ./ 1.46 MiB 99 modules
    modules by path ../../microsoft/kiota-typescript/ 663 KiB
      modules by path ../../microsoft/kiota-typescript/node_modules/ 481 KiB 28 modules
      modules by path ../../microsoft/kiota-typescript/packages/ 182 KiB
        modules by path ../../microsoft/kiota-typescript/packages/http/fetch/dist/cjs/src/middlewares/ 53.6 KiB 20 modules
        modules by path ../../microsoft/kiota-typescript/packages/http/fetch/dist/cjs/src/*.js 37.5 KiB 4 modules
        modules by path ../../microsoft/kiota-typescript/packages/http/fetch/dist/cjs/src/utils/*.js 2.58 KiB
          ../../microsoft/kiota-typescript/packages/http/fetch/dist/cjs/src/utils/...(truncated) 2.16 KiB [built] [code generated]
          ../../microsoft/kiota-typescript/packages/http/fetch/dist/cjs/src/utils/fetch...(truncated) 430 bytes [built] [code generated]
        ../../microsoft/kiota-typescript/packages/abstractions/dist/es/src/index.j...(truncated) 88.5 KiB [built] [code generated]
  + 18 modules
webpack 5.89.0 compiled successfully in 8353 ms

Full bundle size when using Graph.

image

@baywet baywet force-pushed the feature/request-builders-proxy branch from ba9116b to 2b26a0e Compare January 11, 2024 15:29
@baywet
Copy link
Member Author

baywet commented Jan 11, 2024

on the results

@sebastien I'm extremely surprised by those results. Especially the fact the bundle is the same exact size here given we've reduced the amount of code locations in the generated code by half. I could understand we didn't achieve a 50% benefit, but no change at all seems suspicious. I'll try to run a couple of comparisons on my end to see whether I get the same results or not.

on additional things we might want to explore

Query parameters

For query parameters mappings we could change from this

const MessagesRequestBuilderGetQueryParametersMapper: Record<string, string> = {
    "count": "%24count",
    "expand": "%24expand",
    "filter": "%24filter",
    "orderby": "%24orderby",
    "search": "%24search",
    "select": "%24select",
    "skip": "%24skip",
    "top": "%24top",
};

to that

export function MessagesRequestBuilderGetQueryParametersMapper(source: MessagesRequestBuilderGetQueryParameters): Record<string, unknown> {
    return {
        "%24count": source.count,
        "%24expand": source.expand,
        "%24filter": source.filter,
        "%24orderby": source.orderby,
        "%24search": source.search,
        "%24select": source.select,
        "%24skip": source.skip,
        "%24top": source.top,
    };
}

The idea being we'll have fewer strings, but more code locations (35 vs 108). I'm not sure this will result in gain since strings usually compress well. But it might be worth tryings

Request metadata

Currently looks like this

export const MessagesRequestBuilderRequestsMetadata: Record<string, RequestMetadata> = {
    "get": {
        responseBodyContentType: "application/json",
        errorMappings: {
            "4XX": createODataErrorFromDiscriminatorValue,
            "5XX": createODataErrorFromDiscriminatorValue,
        } as Record<string, ParsableFactory<Parsable>>,
        adapterMethodName: "sendAsync",
        responseBodyFactory:  createMessageCollectionResponseFromDiscriminatorValue,
        queryParametersMapper: MessagesRequestBuilderGetQueryParametersMapper,
    },
    "post": {
        responseBodyContentType: "application/json",
        errorMappings: {
            "4XX": createODataErrorFromDiscriminatorValue,
            "5XX": createODataErrorFromDiscriminatorValue,
        } as Record<string, ParsableFactory<Parsable>>,
        adapterMethodName: "sendAsync",
        responseBodyFactory:  createMessageFromDiscriminatorValue,
        requestBodyContentType: "application/json",
        requestBodySerializer: serializeMessage,
        requestInformationContentSetMethod: "setContentFromParsable",
    },
};

We could replace verbs (get/post/...) by an enum, I'm not sure how much strings vs import of an enum is going to make an impact. My guess is that it'll be beneficial to segments with multiple operations, detrimental to single operations.
We could also do some additional micro-optimizations at generation time like: if we see that both 4XX and 5XX are mapped to the same type, generate a single XXX entry.

On tree shaking

After further inspection, our client is still a big tree of the API surface and all its operations. It should be lighter since we have about half the code locations, but it'll still be hard to tree shake at this point.
Besides introducing selective imports, or going all the way to self-serve, I'm not sure we'll be able to do much on that front.

On the initial FHL POC

Here are the reasons why the FHL project is much lighter:

  • It doesn't really deserialize, but simply relies on JSON, which would mean a format portability tradeoff, loosing the discriminator deserialization, etc...
  • It assumes everything is JSON (no accept/content-type headers)
  • It doesn't have error types mapping (or assumes they are global for all endpoints)
  • It assumes all endpoints will support all operations (which is not true)
  • It assumes all GET operations support all query parameters (which is not true)
  • It concatenates URI paths segments instead of relying on URI templates (less duplications, but edge cases when it comes to encoding etc...)

andrueastman
andrueastman previously approved these changes Jan 12, 2024
@baywet
Copy link
Member Author

baywet commented Jan 12, 2024

I'll push additional optimizations soon, but first I wanted to establish a baseline.
What I have: Seb's app without azure identity using the full SDK without core (the idea being to measure what we care about)

image

asset bundle.prod.js 15.6 MiB [emitted] [minimized] (name: main) 1 related asset
orphan modules 32.6 MiB [orphan] 7896 modules
runtime modules 937 bytes 4 modules
built modules 33.1 MiB [built]
  modules by path ./node_modules/ 654 KiB 55 modules
  modules by path ../msgraph-sdk-typescript/ 21.2 KiB
    ../msgraph-sdk-typescript/node_modules/@std-uritemplate/std-uritemplate...(truncated) 16.2 KiB [built] [code generated]
    ../msgraph-sdk-typescript/node_modules/tinyduration/dist/index.js 3.15 KiB [built] [code generated]
    ../msgraph-sdk-typescript/node_modules/guid-typescript/dist/guid.js 1.92 KiB [built] [code generated]
  ./src/index.ts + 7726 modules 32.4 MiB [not cacheable] [built] [code generated]
  external "crypto" 42 bytes [built] [code generated]
  external "punycode" 42 bytes [built] [code generated]
webpack 5.89.0 compiled successfully in 459279 ms

dist.zip

@baywet
Copy link
Member Author

baywet commented Jan 12, 2024

With the proxy instead (latest updates I just pushed), and in the same configuration, here are the results
image

asset bundle.prod.js 8.61 MiB [emitted] [minimized] (name: main) 1 related asset
orphan modules 20.5 MiB [orphan] 7778 modules
runtime modules 937 bytes 4 modules
built modules 21 MiB [built]
  cacheable modules 531 KiB
    modules by path ../kiota-typescript/node_modules/ 437 KiB 27 modules
    modules by path ../kiota-typescript/packages/http/fetch/dist/cjs/src/ 93.6 KiB
      modules by path ../kiota-typescript/packages/http/fetch/dist/cjs/src/middlewares/ 53.6 KiB 20 modules
      modules by path ../kiota-typescript/packages/http/fetch/dist/cjs/src/*.js 37.4 KiB 4 modules
      modules by path ../kiota-typescript/packages/http/fetch/dist/cjs/src/utils/*.js 2.58 KiB 2 modules
  ./src/index.ts + 7667 modules 20.3 MiB [not cacheable] [built] [code generated]
  ../kiota-typescript/packages/abstractions/dist/es/src/index.js + 44 modules 89.1 KiB [not cacheable] [built] [code generated]
  ../kiota-typescript/node_modules/node-fetch/lib/index.mjs + 5 modules 44.4 KiB [not cacheable] [built] [code generated]
  external "punycode" 42 bytes [built] [code generated]
webpack 5.89.0 compiled successfully in 1966390 ms

We can see the bundle went from 15.6MB to 8.6MB ~44% reduction.
The time to compile however went up x3 :(

dist.zip

@baywet
Copy link
Member Author

baywet commented Jan 12, 2024

There are additional optimizations we could implement (not graph specific):

  • deduplicate 5XX and 4XX when they are the same to XXX (this could benefit other languages as well)
  • assume adapterMethodName: "sendAsync" is a default (not emit it, emit only the other cases)
  • replace adapterMethodName: "sendAsync" and requestInformationContentSetMethod: "setContentFromParsable" by numbers for values so we save a lot of strings.
  • assume responseBodyContentType: "application/json" is a default (not emit it, emit only the other cases)

There are additional optimizations we could implement (graph specific):

  • use a set of global default query parameter mappings (and not emit the mappings on a per operation basis)
  • define global error mappings (and not emit those on every operation)

More investigations to follow

@baywet baywet force-pushed the feature/request-builders-proxy branch from 7bf81d7 to b420de2 Compare January 15, 2024 15:51
@baywet
Copy link
Member Author

baywet commented Jan 15, 2024

update: the current form (aside from maybe deduplicating the error mappings as it's a low hanging fruit) should be sufficient enough since we pivoted to splitting the full service library across multiple packages (one for models and empty service client + one per root path segment for fluent API with modular augmentation). Here are the results of an early investigation:

image

asset bundle.prod.js 519 KiB [emitted] [minimized] (name: main) 1 related asset
orphan modules 4.75 MiB [orphan] 148 modules
runtime modules 937 bytes 4 modules
built modules 5.25 MiB [built]
  cacheable modules 531 KiB
    modules by path ../kiota-typescript/node_modules/ 437 KiB 27 modules
    modules by path ../kiota-typescript/packages/http/fetch/dist/cjs/src/ 93.6 KiB
      modules by path ../kiota-typescript/packages/http/fetch/dist/cjs/src/middlewares/ 53.6 KiB 20 modules
      modules by path ../kiota-typescript/packages/http/fetch/dist/cjs/src/*.js 37.4 KiB 4 modules
      modules by path ../kiota-typescript/packages/http/fetch/dist/cjs/src/utils/*.js 2.58 KiB 2 modules
  ./src/index.ts + 37 modules 4.6 MiB [not cacheable] [built] [code generated]
  ../kiota-typescript/packages/abstractions/dist/es/src/index.js + 44 modules 89.1 KiB [not cacheable] [built] [code generated]
  ../kiota-typescript/node_modules/node-fetch/lib/index.mjs + 5 modules 44.4 KiB [not cacheable] [built] [code generated]
  external "punycode" 42 bytes [built] [code generated]
webpack 5.89.0 compiled successfully in 22790 ms

dist.zip

baywet added 13 commits January 19, 2024 10:03
…t for interfaces

Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
- implements request metadata constant

Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
…e the wrong name in typescript

Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
baywet and others added 18 commits January 19, 2024 10:03
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Co-authored-by: Eastman <andrueastman@users.noreply.github.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
…adata

Signed-off-by: Vincent Biret <vibiret@microsoft.com>
- removes strings for typescript navigation metadata

Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
@baywet baywet force-pushed the feature/request-builders-proxy branch from b420de2 to 306a4cf Compare January 19, 2024 15:05
@baywet
Copy link
Member Author

baywet commented Jan 19, 2024

@andrueastman this is ready for final review and merge :)

@baywet baywet enabled auto-merge January 19, 2024 15:08
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
0 Security Hotspots
90.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@baywet baywet merged commit 47d1925 into main Jan 23, 2024
170 of 186 checks passed
@baywet baywet deleted the feature/request-builders-proxy branch January 23, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigation for Proxy generation for Request Builders in TypeScript
3 participants