-
Notifications
You must be signed in to change notification settings - Fork 232
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
Updated specifications for the Graph JS v4 Service Library #1104
base: 4.0.0
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together.
> This example succeeds because `me` is at the root of the Graph Client | ||
|
||
```typescript | ||
const me = await graphClient.me.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should make a different between first level path segments and the rest. This introduces a inconsistency of experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're losing a lot of discoverability if the bare API Client has no references to anything. We're not talking about the self-serve option here, we're expecting the "full" API available. Thoughts @darrelmiller @gavinbarron @nikithauc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're in the realm of trade-offs here. I agree that import hurts discoverability a lot, the benefit is that by being explicit which children of the root node you want to pull in then the produced app bundle becomes much smaller as it's not including root nodes your app doesn't want.
My instinct here is to try the more discoverable approach in the first instance and evaluate the produced bundle size to see if we need to optimize for size over discoverability.
```typescript | ||
import "@microsoft/microsoft-graph-client/users/item"; | ||
const directReports = await graphClient.usersById("ab677c1b-50c6-4013-aa9f-792ed42c59c8").directReports.get(); | ||
const extensions = await graphClient.usersById("ab677c1b-50c6-4013-aa9f-792ed42c59c8").extensions.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/item/extensions hasn't been imported, this should fail too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extensions is a child resource of /users/item, so I expect extensions to be available. Am I seeing this wrong? I'm not asking for a specific extension, but the collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same understanding as Sébastien here. Importing a node of the tree makes all of its direct children available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand the expected behaviour. If we're expecting the first level nodes to be automatically imported for discoverability and child nodes for any nodes to be imported automatically as long as the parent is imported, we're effectively pulling the full tree and not doing any selective imports anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding was that we'd be importing a node and that would make it's children, and no deeper in the tree available, e.g. importing /users
makes /users/{id}
available, but not /users/{id}/directReports
as this would require /users/item
to be imported, which would also make /users/{id}/extensions
available.
Similarly, the root node makes all first level paths available, and in that case that node is already imported implicitly.
const directReports = await graphClient.usersById("ab677c1b-50c6-4013-aa9f-792ed42c59c8").directReports.get(); | ||
const extensions = await graphClient.usersById("ab677c1b-50c6-4013-aa9f-792ed42c59c8").extensions.get(); | ||
const extensions = await graphClient.usersById("ab677c1b-50c6-4013-aa9f-792ed42c59c8").extensionsById("85147e97-ed89-4799-b5b8-8d35a9204604").get(); | ||
const joinedTeams = await graphClient.me.joinedTeams.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for joined teams
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If /users/item is already imported, /me/joinedTeams should be available. Or should we specifically import for /me? I think of /me to be a shortcut for /users/{user-id}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly Kiota has no way of telling that /me and /users/id are aliases, so there needs to be separate imports here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the repetitive nodes aspects is something we've been talking about while investigating the go build performance issues.
Effectively the OpenAPI spec has no way of describing "this node is a clone of that other node". We could get around that by using an extension.
Now the next problem becomes, does the conversion process make the information available? Semantically speaking, because of OData notations, two nodes can be really similar but not identical (ignoring for description and path). For example, a node could support a PUT/PATCH operation in one context but not in the other, or have a OData method attached in only one context.
Assuming that the conversion library solves for that, we now have a deterministic behaviour issue.
E.g. I convert my metadata and generate SDKs in September, at this time /me and everything under it is identical to /users/id and everything under it.
The SDK generates a single set of request builders, inline requests and responses types.
Then in October somebody adds an annotation on /me but not on /users/id. The conversion process judges that those nodes are now different, produces the description accordingly, and Kiota generates two sets of types. Effectively breaking half the applications that had references "from the wrong path".
We could major bump every-time that happens, but with a service the size of Microsoft Graph and the highly repetitive nature of OData I anticipate this is going to be extremely painful for us and for our customers.
So the conclusion of that was not to deduplicate nodes automatically but make conscious and careful decisions when we see opportunities like that using kiota's filtering capability and a little bit of handcrafted code in the service lib. A good example of that is /me/ and /users/id. They overlap 100% today, are huge, and even if they started drifting in the future, we'd keep the deduplication going and tell customers impacted by the edge case that's it's a trade off decision for the size of the SDK.
You can see an example of that here, it's only a couple of minutes of work, yields about a 17% reduction in build time (and probably similar in size, but I haven't measured that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional aspect I forgot to mention is that we could simplify the selective import story in TypeScript by manually maintaining "presets", a little like pnpJS does. Where we'd have on one hand the granular modular augmentation (each node level) generated by kiota, and on the other presets that leverage them.
And examples would be
import "msgraphsdk/presets/user";
import "msgraphsdk/presets/teams";
await client.me.joinedTeams.get();
await client.users.byId("foo").joinedTeams.get();
In that example both me and users are here because of the user preset, and joined teams is available under both of them because of the teams preset.
Co-authored-by: Vincent Biret <vibiret@microsoft.com>
Co-authored-by: Vincent Biret <vibiret@microsoft.com>
Co-authored-by: Vincent Biret <vibiret@microsoft.com>
Co-authored-by: Vincent Biret <vibiret@microsoft.com>
@nikithauc based on the latest progress, can we have an update to these specs, especially around the approach taken for the selective imports? That way we could close this specs PR with the latest engineering decisions. Thanks! |
This is a first stab at providing more context around the v4 service library and the usage of the selective imports capability. I'd love to learn what you feel would be missing, any questions that this doc could help answer and comments on the overall document.