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

Adding models + interfaces #675

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 149 additions & 2 deletions design/kiota-e2e.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,155 @@ Before we jump into the end-to-end walk-through, it's important to set some cons

## Table of Contents

- [NodeJS e2e using the Service library](#node-service-library)
- [NodeJS e2e using the Core library](#node-core-library)
- [Foudation](#foundation)
- [NodeJS e2e using the Service library](#node-service-library)
- [NodeJS e2e using the Core library](#node-core-library)

## <a id="foundation"></a> Foundation
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## <a id="foundation"></a> Foundation
## Foundation

github adds the anchors automatically for you when using headings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't allow me to "know" what will be the generated anchor. So I prefer this for more complex TOCs.

Copy link
Member

Choose a reason for hiding this comment

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

it uses kebab casing Foundation => foundation, My Foundation => my-foundation


This section lists all the foundation elements of the Graph JS SDK and how it affects the developer's experience.

### Models

The models should be easily consumable by developers in a natural format. For JavaScript and TypeScript developers, interfaces are the most common way to understand, represent and send data to endpoints.

Thanks to the TypeScript nature of allowing merging of Classes and Interfaces with the same name, models are represented in a way that allows both rich capabilities of the classes (backing stores, dirty tracking, etc.) and simplicity of using interfaces.
Copy link
Contributor

Choose a reason for hiding this comment

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

Importing a class and interface with the same name gives me this issue.

Try the following:

//File a.ts
export interface name{
    id:string;
    test:string;
}

export class name{

    id:string;
    type:string;
    test:string;

    public getFieldDeserializers():Array<string>{
        return ["name","type"];
    }
}

//b.ts
import {name} from "./a";

const test: name ={ id:"name", type:"type"};

I get a prompt on the test variable that Error: Type '{ id: string; type: string; }' is missing the following properties from type 'name': test, getFieldDeserializersts(2739).

Interfaces can have duplicate names and that works fine in TypeScript because they do not transpile. However, we are not getting rid of the problem that we see today that requires all properties to be set when using a json initialization pattern because the class requires all the properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

We would need to think about the import pattern when using same named interfaces and classes.


```typescript
export class SendMailRequestBody implements Parsable, SendMailRequestBody {
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment mentioning that interfaces and classes might have a different name or live in different packages depending on implementation/runtime constraints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You think?

Copy link
Member

Choose a reason for hiding this comment

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

yup

/** Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well. */
private _additionalData: Map<string, unknown>;
Copy link
Member

Choose a reason for hiding this comment

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

update additionalData to Record @nikithauc is already working on that change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just there are an "example". Didn't mean to replicate the actual implementation.

Copy link
Member

Choose a reason for hiding this comment

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

then add a comment saying the purpose of this snippet is to demonstrate the api surface, not the implementation details. I don't want people to mistake that for a technical spec

private _message?: Message | undefined;
private _saveToSentItems?: boolean | undefined;
/**
* Instantiates a new sendMailRequestBody and sets the default values.
*/
public constructor(value?: Partial<SendMailRequestBody>) {
if(value) {
Object.assign(this, value);
Copy link
Member

Choose a reason for hiding this comment

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

this section might be a bit more complex than that, we need to account for properties that are complex types (message body) or collections of complex types (message to recipients) as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the goal. I Think I'll simply remove the code. This is not meant for the internal implementation, but the way developers use the SDK.

Copy link
Member

Choose a reason for hiding this comment

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

yep, replace by a comment saying the actual implementation object gets instantiated from the partial values

}
this._additionalData = new Map<string, unknown>();
};
/**
* Gets the additionalData property value. Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well.
* @returns a Map<string, unknown>
*/
public get additionalData() {
return this._additionalData;
};
/**
* Gets the message property value.
* @returns a message
*/
public get message() {
return this._message;
};
/**
* Gets the saveToSentItems property value.
* @returns a boolean
*/
public get saveToSentItems() {
return this._saveToSentItems;
};
/**
* The deserialization information for the current model
* @returns a Map<string, (item: T, node: ParseNode) => void>
*/
public getFieldDeserializers<T>() : Map<string, (item: T, node: ParseNode) => void> {
return new Map<string, (item: T, node: ParseNode) => void>([
["message", (o, n) => { (o as unknown as SendMailRequestBody).message = n.getObjectValue<Message>(Message); }],
["saveToSentItems", (o, n) => { (o as unknown as SendMailRequestBody).saveToSentItems = n.getBooleanValue(); }],
]);
};
/**
* Serializes information the current object
* @param writer Serialization writer to use to serialize this model
*/
public serialize(writer: SerializationWriter) : void {
if(!writer) throw new Error("writer cannot be undefined");
writer.writeObjectValue<Message>("message", this.message);
writer.writeBooleanValue("saveToSentItems", this.saveToSentItems);
writer.writeAdditionalData(this.additionalData);
};
/**
* Sets the additionalData property value. Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well.
* @param value Value to set for the AdditionalData property.
*/
public set additionalData(value: Map<string, unknown>) {
this._additionalData = value;
};
/**
* Sets the message property value.
* @param value Value to set for the Message property.
*/
public set message(value: Message | undefined) {
this._message = value;
};
/**
* Sets the saveToSentItems property value.
* @param value Value to set for the SaveToSentItems property.
*/
public set saveToSentItems(value: boolean | undefined) {
this._saveToSentItems = value;
};
}

export interface SendMailRequestBody {
additionalData: Map<string, unknown>;
message: Message | undefined;
saveToSentItems: boolean | undefined;
};
```

When using the core or service library, it's then possible to provide a simple structure that will be equivalent to the `interface` without having to initialize every models within the object structure, similar to how `json` represents data:

```typescript
await serviceclient.me.sendMail.post({
message: {
subject: 'My Subject'
},
saveToSentItems: true
});
```

The different public methods available on the libraries should allow `Partial<Model>` instead of "full" objects to ensure we can pass in only the necessary properties. This will enable developers to pass in complex structure without specifying new objects every time :

```typescript
var mailBody = new SendMailRequestBody();
const recipient = new Recipient();
recipient.emailAddress = new EmailAddress();
recipient.emailAddress.address = "admin@M365x55726300.OnMicrosoft.com";
const message = new Message();
message.subject = "Hello from Graph JS SDK";
message.toRecipients = [recipient];
message.body = new ItemBody();
message.body.content = "Hello World!";
message.body.contentType = BodyType.Text;
mailBody.message = message;
mailBody.saveToSentItems = true;

await serviceclient.usersById(userId).sendMail.post();
```

vs.

```typescript
await serviceclient.usersById(userId).sendMail.post({
message: {
subject: 'Hello from Graph JS SDK',
toRecipients: [
{
emailAddress: 'admin@M365x55726300.OnMicrosoft.com'
}
],
body: {
content: 'Hello World!',
contentType: 'Text'
}
},
saveToSentItems: true
});
```

## <a id="node-service-library"></a> NodeJS e2e using the Service library

Expand Down