microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
3.02k stars 210 forks source link

TS - Adding a barrel, that is, `index.ts` rolling up exports into a single file #938

Closed nikithauc closed 10 months ago

nikithauc commented 2 years ago

The Kiota Generation must also generate a index.ts barrel file which would do the following:

Example: https://github.com/microsoft/kiota/blob/main/http/typescript/fetch/src/index.ts

So the index.ts file would look something like:

export {UserRequestBuilder} from './users/item/userRequestBuilder';
export {Message} from '../../../models/microsoft/graph/message';

The purpose using an index.ts file is that the user of a generated library should be simply able to do a conventional import as follows:

// Example of a `GraphServiceLibrary` use

import {UserRequestBuilder} from "@microsoft/msgraph-sdk-typescript"

instead of (the current case):

import {UserRequestBuilder} from "@microsoft/msgraph-sdk-typescript/users/item/userRequestBuilder"

Thanks to @rkodev who brought this issue to my attention! He faced this issue while Raptor testing. @rkodev Can you please expand on this problem?

A challenge that I can think of while introducing the barrel:

baywet commented 2 years ago

thanks for starting a conversation on this. Kiota already supports doing things like that, we had to implement that for Ruby. It'd only need to be implemented for TypeScript (addition of a CodeNamespaceWriter, and special case for the naming convention in the PathSegmenter).

While I do support this for models (a complete recursive rollup), I'm not sure we should do it for request builders. You stated the naming conflicts, and that would also impact the bundler's ability to treeshake if all the imports in the customer's code are "root imports" on the package (vs selective imports like today).

sebastienlevert commented 2 years ago

I think the goal here is to have models being exported so you can use their types, but will people really use RequestBuilders as types? Or use them as ways to get to their data?

As a dev, I'd do something like this :

import { Message } from '@microsoft/msgraph-sdk-typescript`;

var message: Message = await graphClient.Me.Messages.GetAsync();
console.log(message.title);

I don't know if I'd need the MessageRequestBuilder in this case. And if this is the case, I'd do it in very specific cases, not necessarily with all my request builders and then getting the "right" request builder in the hierarchy might make more sense.

baywet commented 2 years ago

There are multiple aspects to that, I think a better example would be sending a message, which today would look something like this

import { Message } from '@microsoft/msgraph-sdk-typescript/models';
import { SendMailRequestBody } from '@microsoft/msgraph-sdk-typescript/me/sendmail';
import { MessagesRequestBuilder } from '@microsoft/msgraph-sdk-typescript/me/messages';

const message = new Message(); //... set all the properties you want
const body = new SendMailRequestBody(); // set other action parameters
body.message = message;

const result = await graphClient.me.sendMail.post(body);

const page1 = await graphClient.me.messages.get();

const page2 = await (new MessagesRequestBuilder(page1.nextLink, requestAdapter)).get();
  1. models are already flat for the most part, and because the classes/enums are one per file, naming conventions get us here without a barrel. The only exception to that would be namespaced models like education, some sharepoint stuff, intune etc where you'd need to index into the namespace under models.
  2. request bodies, when they are not a model straight (component vs inline definition) live next to their request builders. But I feel like those are easy to discover. when you type graphClient.me.sendMail.post, if you f12 on post, it'll take you to the method definition, and you'll be able to discover where the type lives. Plus the package segmentation is really close to the fluent API path.
  3. you will have access to all the request builders through the fluent API path, the only exception to that being iterating on pages/resuming delta. But we can facilitate that through a page iterator (which I believe we already have for the most part in the JS SDK) and without needing a barrel.

For all those reasons I haven't implemented a barrel to date for typescript. It does feel like its necessity is limited to edge cases we have other ways to solve for, and it could come at a serious drawback in terms of package size because of the import style and its implications on tree-shaking.

nikithauc commented 2 years ago

TS does not support package.json exports - https://github.com/microsoft/TypeScript/issues/33079

baywet commented 2 years ago

Closing as this has been fixed through #1274

nikithauc commented 2 years ago

Reopening this as #1274 partially fixes this.

This feature needs more discussion and may need more work after considering limitations such as https://github.com/microsoft/TypeScript/issues/33079

nikithauc commented 2 years ago

Documentation should be available on how to manage exports.

baywet commented 1 year ago

@sebastienlevert, can you provide a rationale please?

sebastienlevert commented 1 year ago

We were under the impression this was already done. Reopening then.

baywet commented 1 year ago

Some of it is done, but I think enums are missing, order in indeterministic, and something else was missing last time I checked

sebastienlevert commented 1 year ago

This actually seems connected to the service library which is currently on hold... As we wouldn't need to do this for self-serve libraries. Am I missing something? Or do we want to generate a barrel file for self-serve also? @baywet

baywet commented 1 year ago

generation result is exactly the same between self-serve and service lib. Only the scope of the OpenAPI definition changes and some other parameters (namespace/client name, etc...). This is something I've been pushing back on: having a "special generation" just for service libs. It would create differences of experiences between the two and increase maintenance burden.

sebastienlevert commented 1 year ago

Totally agree that it would be a mess. Now, do we need the barrel in self-serve / kiota-only mode?

baywet commented 1 year ago

I think we do to ease up the import story

without barrels

import { User } from 'graph/models/user';
import { Group } from 'graph/models/group';

with a barrel

import { Group, User } from 'graph/models';
sebastienlevert commented 1 year ago

I thought we already had that work. Ok, let's keep it.

baywet commented 1 year ago

We have it for classes/interfaces, but the order is indeterministic which makes reviews harder. And I think we're missing it for enums

baywet commented 1 year ago

Enums are done. We should validate whether the barrels order is deterministic at this point (run multiple code-gens in v1 full, see if the content of the barrels changes, if it does, fix it)

sebastienlevert commented 1 year ago

We are leaving it for post-GA for reconsideration. Current state is good enough.

baywet commented 10 months ago

a little update on this one:

And rolling everything into a single barrel (models and sub-modules, fluent API paths and submodules) would lead to name collisions, so we won't be doing the last part. Closing.