Adding interfaces to TypeScript generation for models, request configs and query params

[nikithauc]

fixes #1013 fixes #1521

Generated samples : https://github.com/microsoft/kiota-typescript/tree/nikithauc/sample-interfaces/packages/test/generatedCode/models/microsoft/graph

Graph v1 generation : https://github.com/microsoftgraph/msgraph-sdk-typescript/tree/nikithauc/sample-interfaces/src

[nikithauc]

Converting this to draft for now as I am investigating a bug where a some request executor method body parameters are not being replaced in the Graph V1 scope.

[nikithauc]

https://github.com/microsoftgraph/msgraph-sdk-typescript/actions/runs/2368809346 - Graph V1 SDK successful runtime action

[nikithauc]

The sonar cloud comments have been resolved.

[baywet]

thanks for making most of the changes. Could you also please:

• add a changelog entry
• put together a PR on the kiota-samples repo to update the typescript samples

[nikithauc]

thanks for making most of the changes. Could you also please:

• add a changelog entry
• put together a PR on the kiota-samples repo to update the typescript samples

done

[baywet]

Thanks for making the changes. I still believe we're missing an aspect: properties that are complex types or collections of complex types. if an event has a start date property of type date time time zone (also a model) when the JSON build object gets passed to it's constructor, if we have a value for that property, we should call the constructor for date time time zone and pass the value.

[nikithauc]

Thanks for making the changes. I still believe we're missing an aspect: properties that are complex types or collections of complex types. if an event has a start date property of type date time time zone (also a model) when the JSON build object gets passed to it's constructor, if we have a value for that property, we should call the constructor for date time time zone and pass the value.

Tried applying this suggestion. But this approach errors with the call stack exceeding error. I am afraid this introduces circular dependencies when initializing looped classes.

The serialization part already takes care of creating an instance. But doing that in the constructor errors out.

https://github.com/microsoftgraph/msgraph-sdk-typescript/runs/7083070579?check_suite_focus=true#step:6:2454

I will open a new issue to test some scenarios based on this comment. It is a valid comment and the generation should securely map the code everytime.

[baywet]

thanks for looking into that, setting the PR back to draft for now until we figure out a way out of circular dependencies

[nikithauc]

The issue can addressed separately and not necessarily in this PR. The current PR works fine at runtime for the Graph v1 generation.

Also, the issue is not introduced in this PR. I believe it is an issue in the current generation process in use cases with longer chains and bulky sizes. I faced the issue for the Graph SDK and not for a smaller chunk.

[baywet]

Unless I missed something I don't believe properties that are complex types or collections of complex types are handled yet. If they are, can you also update the samples pr please?

[nikithauc]

Unless I missed something I don't believe properties that are complex types or collections of complex types are handled yet. If they are, can you also update the samples pr please?

Unless I missed something I don't believe properties that are complex types or collections of complex types are handled yet. If they are, can you also update the samples pr please?

https://github.com/microsoft/kiota-samples/pull/776/files#diff-1b6ef19468094e83c07715397cc16851c9f69259095c5a2a17504e41ab30f68cR74

https://github.com/microsoft/kiota-samples/pull/776/files#diff-6ffb85a3bf255b048e48eb5b7c799751e9d030081fd7773f1d793a0995fa9f9dR42

This is how a complex type is handled at the serialization step.

[baywet]

Thanks for sharing that, I still think it should be done at the constructor level instead so we don't loose things like backing story dirty tracking on the properties of those complex type properties.

[nikithauc]

Thanks for sharing that, I still think it should be done at the constructor level instead so we don't loose things like backing story dirty tracking on the properties of those complex type properties.

Adding that at the constructor level caused the issue stated in #1669.

We will still need to maintain this at the serialization level because:

• We get a compile time error as the serializer expects a class instance, but the property is of a model interface type.
• Keeping the following use case, where properties are initialized outside the constructor, in mind:

const user = new UserImpl(); 
user.Address = {city:"", street: ""}
s.

[baywet]

why does the model expect a class instance? the generic type constraint only specifies parsable. https://github.com/microsoft/kiota-typescript/blob/91a90e3a71e0dfe93386ddb2c1e1ff540e9c9f1e/packages/abstractions/src/serialization/serializationWriter.ts#L91

The other affectation case you've shared is valid, I didn't think of that, thanks for sharing it.

What if we move that logic into the setter then instead? it should both satisfy the constructor and the affectation cases?

[nikithauc]

why does the model expect a class instance? the generic type constraint only specifies parsable. https://github.com/microsoft/kiota-typescript/blob/91a90e3a71e0dfe93386ddb2c1e1ff540e9c9f1e/packages/abstractions/src/serialization/serializationWriter.ts#L91

The other affectation case you've shared is valid, I didn't think of that, thanks for sharing it.

What if we move that logic into the setter then instead? it should both satisfy the constructor and the affectation cases?

1. Example

  writer.writeObjectValue<MessageRulePredicatesImpl>("exceptions", this.exceptions!);

Error : Argument of type 'MessageRulePredicates' is not assignable to parameter of type 'MessageRulePredicatesImpl' as a class instance may not necessarily be of a type or another implementation of the same interface.

2. Regarding a setter, the code still needs to map each property value of the interface based object to the class instance property which causes the issue #1669 in the Graph V1 SDK.

This PR is the first iteration.

I expect more work to the models interfaces-classes aspects based on:

• #1669
• #1662

[nikithauc]

why does the model expect a class instance? the generic type constraint only specifies parsable. https://github.com/microsoft/kiota-typescript/blob/91a90e3a71e0dfe93386ddb2c1e1ff540e9c9f1e/packages/abstractions/src/serialization/serializationWriter.ts#L91 The other affectation case you've shared is valid, I didn't think of that, thanks for sharing it. What if we move that logic into the setter then instead? it should both satisfy the constructor and the affectation cases?

1. Example

  writer.writeObjectValue<MessageRulePredicatesImpl>("exceptions", this.exceptions!);

Error : Argument of type 'MessageRulePredicates' is not assignable to parameter of type 'MessageRulePredicatesImpl' as a class instance may not necessarily be of a type or another implementation of the same interface.

2. Regarding a setter, the code still needs to map each property value of the interface based object to the class instance property which causes the issue TypeScript - Investigate if nesting model class instances causes circular dependency #1669 in the Graph V1 SDK.

This PR is the first iteration.

I expect more work to the models interfaces-classes aspects based on:

• TypeScript - Investigate if nesting model class instances causes circular dependency #1669
• Create spec for TypeScript model usage using interfaces and classes.  #1662

Adding on the use of a setter, let's take the following example:

interface IUser
{
  drive :Drive;
}

 class UserImpl implements IUser{ 
    private _drive: Drive;
    public get drive() :Drive{
        return this._drive;
    }
    public set drive(value: Drive) {
    this._drive = value; 
  }
}

const userInstance = new UserImpl();

const userObject: IUser = {
  drive : new Drive()
}

console.log("Object : =>")
console.log(userObject);
console.log("Instance : =>")
console.log(userInstance);

Output:

[LOG]: "Object : =>" 
[LOG]: {
  "drive": {}
} 
[LOG]: "Instance : =>" 
[LOG]: UserImpl: {
  "_drive": {}
} 

The property in the class instance and object differs. As the get/set does not persist in the JS instance, there is inconsistency between the model interface object and model instance.

This is an issue when working with the model objects and instances. For example, when working with a task like Page Iteration, the code would have to access both response["values"] || response["_values"].

I remember @sebastienlevert raising this issue of the properties starting with "_" but the expectation is to not have them.

[baywet]

On the interface in serialization aspect, what happens with

writer.writeObjectValue<MessageRulePredicates>("exceptions", this.exceptions!); //using the interface as the generic type parameter

(that assuming the properties of the class are using interfaces as well, not the classes)

As per the setter aspect, are you inferring the following will not be using the setter? Or did I misunderstand the example?

const userObject: IUser = {
  drive : new Drive()
}

[nikithauc]

Added model instantiation at the setter level

[baywet]

Thanks for making the updates. I'm not sure why we have this block? https://github.com/microsoft/kiota-samples/pull/776/files/e21410bd705c5b182669b6e006693cd6821c6740..17dc949f99bfffc39fa4df1c99d36f8cfbe436ac#diff-06d4b04f846a062595d3f4c5a08d612f6e15d590a38991b97a1a0e4d151e40eaR74

And why is additional data nullable now? https://github.com/microsoft/kiota-samples/pull/776/files/e21410bd705c5b182669b6e006693cd6821c6740..17dc949f99bfffc39fa4df1c99d36f8cfbe436ac#diff-06d4b04f846a062595d3f4c5a08d612f6e15d590a38991b97a1a0e4d151e40eaR9

[nikithauc]

Thanks for making the updates. I'm not sure why we have this block? https://github.com/microsoft/kiota-samples/pull/776/files/e21410bd705c5b182669b6e006693cd6821c6740..17dc949f99bfffc39fa4df1c99d36f8cfbe436ac#diff-06d4b04f846a062595d3f4c5a08d612f6e15d590a38991b97a1a0e4d151e40eaR74

Which block are you referring to?

And why is additional data nullable now? https://github.com/microsoft/kiota-samples/pull/776/files/e21410bd705c5b182669b6e006693cd6821c6740..17dc949f99bfffc39fa4df1c99d36f8cfbe436ac#diff-06d4b04f846a062595d3f4c5a08d612f6e15d590a38991b97a1a0e4d151e40eaR9

I came across compilation issues with required additional data type . Example:

interface ITest{
    additionalData?:string
}

class test implements ITest{
    additionalData : string

    constructor(test:ITest){
        this.additionalData = test.additionalData
    }
}

Error: Type 'string | undefined' is not assignable to type 'string'.
  Type 'undefined' is not assignable to type 'string'

Also, when creating a request body using the model class, the additional data properties are not set. The additional data property has to be nullable especially when working with interfaces as the additional data becomes required at the time of populating a model object if it is not nullable

[baywet]

this

if(this.value && this.value.length != 0){        const valueArrValue: AttachmentImpl[] = [];
        this.value?.forEach(element => {
            valueArrValue.push((element instanceof AttachmentImpl? element as AttachmentImpl:new AttachmentImpl(element)));
        });

Why is the property defined as nullable in the model interface since it's not in the abstractions interface? https://github.com/microsoft/kiota-typescript/blob/04729081465baa983eec18d72f6853143c016f62/packages/abstractions/src/serialization/additionalDataHolder.ts#L7

Additional data should be set by the constructor.

[nikithauc]

Why is the property defined as nullable in the model interface since it's not in the abstractions interface? https://github.com/microsoft/kiota-typescript/blob/04729081465baa983eec18d72f6853143c016f62/packages/abstractions/src/serialization/additionalDataHolder.ts#L7

Additional data should be set by the constructor.

1. If the additionalData is required in the model interface then every time a user creates a user object an error will be prompted asking for additionalData.

  const message: Message = {subject: "test"}; // This prompts an error if additionalData is required and not set here. 
``
If set as required,

const message = {subject: "test", additionalData: {}}; // This is not ideal

2.
> this
> 
> ```ts
> if(this.value && this.value.length != 0){        const valueArrValue: AttachmentImpl[] = [];
>         this.value?.forEach(element => {
>             valueArrValue.push((element instanceof AttachmentImpl? element as AttachmentImpl:new AttachmentImpl(element)));
>         });
> ```


Added this since compilation fails without the proper casting. 

[baywet]

1. right, could we update the additional data holder interface in the abstractions to align things then please?
2. if we changed this writer.writeCollectionOfObjectValues<AttachmentImpl>("value", valueArrValue); to that writer.writeCollectionOfObjectValues<Attachment>("value", valueArrValue); couldn't we do away with the block? (trying to avoid increasing the size)

[nikithauc]

1. right, could we update the additional data holder interface in the abstractions to align things then please?
2. if we changed this writer.writeCollectionOfObjectValues<AttachmentImpl>("value", valueArrValue); to that writer.writeCollectionOfObjectValues<Attachment>("value", valueArrValue); couldn't we do away with the block? (trying to avoid increasing the size)

For point 2,

I run into compile time errors

Types of property 'getFieldDeserializers' are incompatible.
    Type '(() => Record<string, (node: ParseNode) => void>) | undefined' is not assignable to type '() => Record<string, (node: ParseNode) => void>'.

But you bring up a valid point here. Created https://github.com/microsoft/kiota/issues/1730

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

90.6% Coverage
0.0% Duplication

[baywet]

1. Thanks for making the change!
2. I believe this is because we have Partial<Parsable> which makes the method optional. We could get around that by updating the prototype of GetObjectValue, GetObjetCollectionValue in ParseNode I think.
3. I've noticed that the base types interfaces re-define the additionalData property, which is shouldn't since it's already defined in additional data holder, maybe add a filter in the refiner that creates the interfaces? https://github.com/microsoft/kiota-samples/blob/17dc949f99bfffc39fa4df1c99d36f8cfbe436ac/msgraph-mail/typescript/src/models/entity.ts#L5
4. The samples PR is conflicting for a lot of files. Maybe rebase interactively dropping your commits and re-generate?

[nikithauc]

2. I believe this is because we have Partial which makes the method optional. We could get around that by updating the prototype of GetObjectValue, GetObjetCollectionValue in ParseNode I think.

Not in this PR. I will this update the based on https://github.com/microsoft/kiota/issues/1732 and more design changes.

Also note, this PR is already adding size optimization by replacing config param classes by interfaces.

3. I've noticed that the base types interfaces re-define the additionalData property, which is shouldn't since it's already defined in additional data holder, maybe add a filter in the refiner that creates the interfaces?

This is a separate issue. The changes in this PR are not redefining the additionalData property.

https://github.com/microsoft/kiota-samples/pull/776/files#diff-c9e4b2665b0a69f9959239648d9cc049770afd6b01f23154d8f296bf00c364e6L3

[baywet]

Are you able to provide numbers on the size reduction? I believe that because the classes end up being pulled by the fluent API surface for request bodies and by the discriminators for return types, and because the shape of the fluent API right now is hard to tree shake, it most likely makes only a little difference. Unless I'm missing something?

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

90.6% Coverage
0.0% Duplication

[nikithauc]

Are you able to provide numbers on the size reduction? I believe that because the classes end up being pulled by the fluent API surface for request bodies and by the discriminators for return types, and because the shape of the fluent API right now is hard to tree shake, it most likely makes only a little difference. Unless I'm missing something?

The reduction is in the unpacked disk size of the library because of using interfaces instead of classes. The lib size went from 64mb in preview 1 to 98 mb mainly with the request configurations. Converting the request configurations to interfaces removes the javascript transpiled classes and thereby reducing The bundle size optimization will require a drastic refactoring. At this point the aim is to optimize both the library size and the bundleable size of the fluent api.