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

TypeScript - Use of interface models instead of class models #1013

Closed
nikithauc opened this issue Jan 10, 2022 · 15 comments · Fixed by #1559
Closed

TypeScript - Use of interface models instead of class models #1013

nikithauc opened this issue Jan 10, 2022 · 15 comments · Fixed by #1559
Assignees
Labels
TypeScript Pull requests that update Javascript code WIP
Milestone

Comments

@nikithauc
Copy link
Contributor

Definitely, this would add a lot of value here. But, why are we using classes vs Interfaces? Interfaces would allow us to achieve this. There is probably a reason behind this design, so if we can do the partial initializers, this would be a great start. But let's think in a "JavaScript / TypeScript" way and not in a non-familiar way for developers. If they never used new EmailAddress() in the past, I don't know if they should in the future, as long as they respect the interface...

Creating a separate issue that branches of from the comment made by @sebastienlevert in 841#issuecomment-1007434306

The above comment has been backed by @waldekmastykarz as follows:

+1 to what @sebastienlevert said. new Message() doesn't seem like anything I've ever seen in JavaScript. It's intuitive in C#, but not in JavaScript. Rather than imposing a specific usage patterns on developers, we should see if there are other ways for us to implement necessary plumbing while allowing developers to use programming patterns they're used to.

I agree with the points made here that:

Interfaces are conventionally used for representing models in TypeScript/JavaScript.
The biggest advantage of using ( and a very important reason for using) interfaces is that they don't transpile and generate smaller code. Using interfaces will drastically reduce the size of the transpiled and the bundled code from a generated library.

@baywet based on your reply to the conversation:

because of the way the serialization works.
https://github.com/microsoft/kiota-samples/blob/763939eba29c9a13e9455d44f84abe76c0d44d3a/msgraph-mail/typescript/src/models/microsoft/graph/message.ts#L314
And also in anticipation with backing store.

  • Can you please explain why would the serialization process work requires a class?
  • Can we try upgrading the serialization process to construct/deconstruct interfaces?
  • Can you please elaborate on the point And also in anticipation with backing store?

Related issue: ​
#1008

The problem with the constructor accepting partials as suggested in the issue #1008 is that this approach loses the compile/type safety. This does not look like an optimal option to me.

@nikithauc nikithauc self-assigned this Jan 10, 2022
@nikithauc nikithauc added TypeScript Pull requests that update Javascript code generator Issues or improvements relater to generation capabilities. labels Jan 11, 2022
@baywet
Copy link
Member

baywet commented Jan 11, 2022

The problem with the constructor accepting partials as suggested in the issue #1008 is that this approach loses the compile/type safety. This does not look like an optimal option to me.

Can you expand on why you believe we'd be losing compile time checks with this approach?

Can you please explain why would the serialization process work requires a class?

Kiota generated models rely on auto-serialization to be format/library agnostic. The thinking is the following: each model already knows how to serialize/deserialize itself since all the required information is present in the OpenAPI description. Rather than relying on annotations that'd tie the model to a specific library and format, each model class contains two methods describing how to serialize/deserialize itself which are called by the serialization writer/parse node (and call in turn method from those implementers to get to/from the concrete format).
We could imagine using static methods instead by naming conventions, this would require significant refactoring.

Can we try upgrading the serialization process to construct/deconstruct interfaces?

I believe the limiting factor here, besides the added complexity of using static methods, will be the backing store.

Can you please elaborate on the point And also in anticipation with backing store?

We do have some documentation, but re-reading it, it only documents the "what" not the "why". In general the idea behind the backing store is to store the values behind any given model "somewhere else" than the traditional field/properties a model object could provide.

This unlocks a number of scenarios, the most immediate one behind dirty tracking for objects reuse. Imagine the following:

  1. I get a user.
  2. I update a property on the user object that was returned by the fluent API, let's say the last name.
  3. I update the user with the fluent API using this object.

Without dirty tracking, the client won't be able to tell which properties came back from the API during the initial call, and which ones were updated by the application. The result being the client will send all the properties it has for that object instead of the updated ones to the API, which will lead to undesired behaviors. The dotnet/java SDKs have been suffering from that side effect from years now, and until the backing store, we didn't have a better solution than "use a new object to make your updates", less than ideal from a usability perspective.

The other scenarios are linked to efforts that are under NDA for Microsoft, I'll start a thread on Teams to describe that.

Now there are essentially two main ways of maintaining and synchronizing a state to a third party store, we can look at ORMs because a lot of them do that.

  1. A global context object that track the state of each object making a copy of them when they come back from the database/the application and using their in memory reference. This has the advantage of leaving the model objects "untouched" but adds a lot of complexity to the context object (the client in our case), adds memory pressure (maintaining copies), and becomes a nightmare for the implementer when people are working on multiple contexts and/or threads.
  2. A backing store on each model object. Essentially each model is responsible for carrying its backing store/context, which has the drawback of "impacting" the model objects but brings the benefits of thread safety, decreased memory usage and a lower overall complexity.

I hope this brings clarity CC @zengin on the context/backing store implementations in case I missed another way of implementing that aspect.

Inheritance

Another reason worth pointing out that wasn't mentioned of why we're using classes it because a lot of our models inherit each other (although this could be achieved through interfaces)

Initialization

Another reason why we have classes rather than interfaces is for object initialization. One of the things we initialize in the constructor is the backing store (when used), but we also initialize properties default values (when provided by OpenAPI and when different from the default runtime value), as well as the additional data manager. If we switches to interfaces, the consumer would have to initialize that themselves rather than having the constructor do it for them, or call a factory. Non of those options would be an improvement compared with the current solution IMHO

@waldekmastykarz
Copy link
Contributor

This unlocks a number of scenarios, the most immediate one behind dirty tracking for objects reuse. Imagine the following:

From the point of view developers using the SDK, would they be able to do:

const me = client.me.get();
me.name = 'New name';
me.update();

our would they rather do:

const me = client.me.get();
me.name = 'New name';
client.me.update(me);

In the first case, working with classes makes perfect sense, because you update the object you retrieved from the API and call the update method on it.

In the latter example, if you need to pass the payload to the update method, why not just pass an object with the few properties you want to update?

const me = client.me.get();
client.me.update({
  id: me.id,
  name: 'New name'
});

or even like this if you know the ID upfront (eg. because you retrieved it previously) and don't need to retrieve the entity from the API first:

client.me.update({
  upn: 'mail@contoso.com',
  name: 'New name'
});

In this use pattern, having to use a class seems like an unnecessary hurdle because all properties that are set are new/dirty:

client.me.update(new User() {
  upn: 'mail@contoso.com',
  name: 'New name'
});

So it all comes down to how we want to expose our SDK to developers.

Another reason why we have classes rather than interfaces is for object initialization. One of the things we initialize in the constructor is the backing store (when used), but we also initialize properties default values (when provided by OpenAPI and when different from the default runtime value)

Do you have an example of an entity for which using a constructor would have a clear benefit over an interface and where we'd do some of the extra work for developers?

@baywet
Copy link
Member

baywet commented Jan 12, 2022

Thanks for providing examples.

const me = client.me.get();
me.name = 'New name';
me.update();

Was never an option as this approach would mix the modeling and the request building concerns.

Assuming we have a static method for serialization/deserialization instead of an instance method, and assuming we don't have a backing store, this could work.

const me = client.me.get();
client.me.update({
  id: me.id,
  name: 'New name'
});

Where the code-gen would produce something like update (Partial<IUser> user, ...) and inside of that method's body it would have a a reference to a static method that handles serialization of IUser.

That only works however because you removed the need for dirty tracking, hence the need for an in memory backing store. But there are other scenarios for backing stores I have shared on teams. (let me mention you there).

But overall, would it be better than doing:

const me = client.me.get();
me.name = 'new name';
client.me.update(me);

My OOP background would prefer the later, but maybe I'm biased here.

@sebastienlevert
Copy link
Contributor

sebastienlevert commented Jan 17, 2022

I've been playing around to understand a little bit more the issue and I was able to (I think) bring the conversation a little bit further. Here are a couple of thoughts I had while building this repro : Repro on TypeScript Playground

I strongly believe Types and Models should be somehow decoupled. I'm thinking when using Core library for any type of SDK, users will want the autogenerated fluent APIs, others will just care about the types. Mixing these 2 seems like a limitation for our audience. This allows for a scenario like the following

// Note that we are not using User, but IUser. The User class would implement IUser.
let meFromFetch = await fetch('https://graph.microsoft.com/v1.0/me', { ... }) as IUser;
let meFromCore = client.api('https://graph.microsoft.com/v1.0/me') as IUser;

Now, if we decouple these 2 things and make them work together, it's possible to leverage classes when we need them and interfaces when we need them. To bring back the example described by @baywet above, we could do build for both scenarios (or at least, in Javascript).

When using Request Builders, they would return an instance of the class, making it super easy for anyone to just update what they want, and then ask the API to update with the change tracking it needs

export class User implements IUser {
    ....
}

const me: User = client.me.get();
me.name = 'new name';
client.me.update(me);

If I want to create a fully brand new object (like sending an email), I could either use the Class approach if this suits me, or I could just use the interface to make it easy for Javascript developes to use our types. To accomplish this, the Request Builders would accept an IMessage as a parameter and could validate if we're with an interface or with an actual instance. The underlying classes would implement the Partial<IMessage> constructor and could let partial objects to be passed in.

// Using the class approach
const messageFromClass = new Message({
        'subject': "Subject from Class",
        'body': new ItemBody({
            'contentType': BodyType.Html,
            'content': "<bold>Hello World!</bold>"
        }),
        'toRecipients': [new Recipient({
            'emailAddress': new EmailAddress({
                'address': 'admin@m365x263716.onmicrosoft.com'
            })
        })]
    });

client.me.sendMail(messageFromClass);

// Using the interface approach
const messageFromInterface = {
    'subject': "Subject from Interface",
    'body': {
        'contentType': BodyType.Html,
        'content': "<bold>Hello World!</bold>"
    },
    'toRecipients': [{
        'emailAddress': {
            'address': 'admin@m365x263716.onmicrosoft.com'
        }
    }]
};

client.me.sendMail(messageFromInterface);

With this, I believe we are covering both solutions and are offering a developer experience that is expected from both heavy TypeScript developers and Javascript developers.

@baywet
Copy link
Member

baywet commented Jan 18, 2022

We could generate interfaces in addition to the classes we're already generating if you think it'd be valuable.
This would have a significant impact on the size of the library. Also please answer this question: is adding a property to an existing interface a breaking change or not?

Also, for this to work:

// Using the interface approach
const messageFromInterface = {
    'subject': "Subject from Interface",
    'body': {
        'contentType': BodyType.Html,
        'content': "<bold>Hello World!</bold>"
    },
    'toRecipients': [{
        'emailAddress': {
            'address': 'admin@m365x263716.onmicrosoft.com'
        }
    }]
};

client.me.sendMail(messageFromInterface);

The generation would need to add a reference to some kind of static serialization method in the body of "sendMail", and this static method is currently an instance method on the class. Not impossible, just adding additional weight to the thing.

@baywet
Copy link
Member

baywet commented Jan 18, 2022

It's also interesting to note that in order to get auto-completion most people would probably do

// Using the interface approach
const messageFromInterface = {
    'subject': "Subject from Interface",
    'body': {
        'contentType': BodyType.Html,
        'content': "<bold>Hello World!</bold>"
    },
    'toRecipients': [{
        'emailAddress': {
            'address': 'admin@m365x263716.onmicrosoft.com'
        }
    }]
} as IMessage;

client.me.sendMail(messageFromInterface);

Which makes it really close to the constructor approach.

@nikithauc
Copy link
Contributor Author

  • Instead of using the backing store through the object, we could think of
    associating a singleton backing store with the client instance and update which would look like something as follows:
const userObject = client.store.get(key);
userObject = {
    name : "updated"
}
client.store.update(meObject); 

The backing store instance will track the object.

This would have a significant impact on the size of the library. Also please answer this question: is adding a property to an existing interface a breaking change or not?

We should consider appending the optional "?" to the parameters as we do in typings today.

@baywet
Copy link
Member

baywet commented Jan 20, 2022

This is the alternative I was mentioning during the meeting we had. The context pattern. It doesn't have to be a singleton, that's a separate concern. The trouble with that pattern is that it adds a lot of complexity, you have to maintain indices per collection, what happens when objects get deleted? what happens if the app works with multiple clients to the same API? to different APIs? how do you know which field is the key of the object? (could add an additional property in the conversion process) etc...

This is effectively what most ORMs like Entity Framework are doing, and it's a whole other game. The advantage of having one backing store per model instance is that we don't need any of that.

All that being said, I believe we can achieve Seb's requirements, while still using classes, and while not enforcing the constraint of having to call the constructor to users, I'll detail additional information in #1008 and I'll take a stab at it (at least prototyping) as soon as I have a little bit of time.

@sebastienlevert
Copy link
Contributor

If we can mix both interfaces and classes to deliver the requirements, I'm all for it. Goal is to provide a natural behavior to our Javascript developers. The way classes are in Typescript, I don't believe we're achieving this. Let's wait for your prototype @baywet and we can take it from there!

@waldekmastykarz
Copy link
Contributor

@sebastienlevert following your example of updating information about the current user (me), how would that work if I didn't want to retrieve the information first, just override what's there? Would I be able to do the following or would there be more work involved to wire up tracking the changes and linking the instance of the class to the entity in Graph?

const me = new User();
me.name = 'new name';
client.me.update(me);

@baywet
Copy link
Member

baywet commented Jan 24, 2022

@waldekmastykarz this is how it works with the current generation in Kiota.
My goal, once I have time to sit down and prototype what I have describe it to allow you to do the following for the scenario you describe.

client.me.update({name: 'new name'});

@nikithauc
Copy link
Contributor Author

#1008 (comment)

Are the conversation above and this issue two separate issues?

@baywet
Copy link
Member

baywet commented Jan 26, 2022

@nikithauc thanks for pointing this out, I believe we're talking about the same thing at this point where:

  • each model would also get an interface
  • each model would accept a partial of its interface as a parameter to their constructor
  • in the body of each model for the constructor, we'd have logic that detects the presence of properties that are arrays of models or models to call their constructors in cascade
  • the parameters for the fluent APIs would switch to accept partial of interfaces, and pass the value to the model constructor to hide the complexity away from sdk users
  • the return values of the fluent APIs would still return the model classes instances, but these would implement the interface, so it'd be transparent to the user.

Again, I have to find time to work on that, but if we agree on that potential solution, I'll go ahead, close #1008 and assign this issue to myself.

Good with all of that?

@baywet baywet added this to the Community Preview milestone Feb 11, 2022
@baywet baywet added this to Kiota Feb 18, 2022
@baywet baywet moved this to Todo in Kiota Feb 18, 2022
@baywet baywet removed this from Kiota Mar 3, 2022
@baywet baywet removed the generator Issues or improvements relater to generation capabilities. label Mar 3, 2022
@baywet
Copy link
Member

baywet commented Mar 3, 2022

update on this #1174 added support in the generator for interfaces and a common refiner to "extract" interfaces from models (used in go). Getting closer to this, should we choose this way since #1274 fixes the circular dependencies issue without requiring introduction of interfaces.

@sebastienlevert
Copy link
Contributor

I would leave it open. We still have the issue where it's not natural to utilize classes for TS of JS devs. It's also matters in the size of the runtime.

Let's discuss this on our next sync!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TypeScript Pull requests that update Javascript code WIP
Projects
None yet
5 participants