PHP API Generation Upgrade

[ashneilson]

This is a partial re-write of the JS API Client Library to utilize a new JS Generator from the PHP API Project.

Included are fully JSDoc compliant comments for all Classes, Functions, Properties and Types.

TS Typings are Auto-Generated from the JSDoc comments and then bundled for Type-Script consumers to use.

[ashneilson]

Hey @ashneilson . Currently, I'm using a property on the UserAccountModel called "hasPinCode" which seems to only be exposed on the private "_json" property. Is there any chance we can make this public?

@josh-biddick Sorry! I have no idea why, but I completely forgot to include the Non-Persisted Properties from the Model Schema in the JS Library Compiler!

Now resolved so you'll notice the hasPassword and hasPinCode properties are available on the UserAccountModel.

[ghost]

Hey @ashneilson . The last comment I added, I somehow approved the PR and not sure how to undo an "approve". I think we are pretty much there, I would like however just clarify that my previous comment about Object Destructuring and Interfaces doesn't present any problems.

[ashneilson]

Hey @ashneilson . The last comment I added, I somehow approved the PR and not sure how to undo an "approve". I think we are pretty much there, I would like however just clarify that my previous comment about Object Destructuring and Interfaces doesn't present any problems.

@josh-biddick No trouble man!

I've completed the fix for Optional Query Parameters. There is already an issue open on TypeScript relating to this: microsoft/TypeScript#39906

For now I have defined new types so the properties can be optional.

[ashneilson]

@tainoNZ @josh-biddick I'm ready for this to be merged. Would be great if you both could review and provide any final comments.

[ghost]

npm run build:browser no longer exists, Does it need to be changed to npm run build?

[ashneilson]

@josh-biddick Great spotting mate!

Yes this should be changed to npm run build now 👍

[ghost]

Hey @ashneilson,

I have a few questions in regard to the way we use and structure objects in the API.

Models

My first question is about Models. I see that many of the Models we use are simply JS classes which control access via "getter" methods to access thier private properties i.e. Site.name

One issue I'm currently experiencing is in the case where we want to combine two objects together. If we were working with standard POJO's (Plain Old Javascript Objects) for example, we could do something like this:

  const site = new Site();
  const siteWithAddedProperties = {
      ...site, <-- via spread operator 
    companyName: 'RICADO',
  };

However, the spread operator does not copy across public getter methods. Instead, when working with objects that have getters/setters, instead we would need to write something like this:

    const site = new Site();
    const siteWithAddedProperties = { 
      id: site.id, 
      name: site.name, 
      ... <- every member of site needs to be defined manually
    };
    siteWithAddedProperties.companyName = 'RICADO'

We can alternatively create a clone of the original site:

    const site = new Site();
    const siteWithAddedProperties = Object.assign(clone(site), { companyName: 'RICADO' })

Unfortunately Object.assign will instead set the property onto the newly cloned version of site. Meaning, that it's property accessors remain the same. Therfore, we couldn't then set new values because the original object only contained getter accessors.

Interfaces

The ability to use Interfaces to share common structure between two similar objects would be fantastic. For example, creating a new SiteDisplay object which extends the properties of Site.

interface ISiteDisplay extends ISiteModel {
  companyName: string;
}

Currently, because classes are being used in place of Interfaces, we must extend the Models.[ModelName] definitions as such:

interface ISiteDisplay extends Models.SiteModel {
  companyName: string;
}

This creates an issue, because our new ISiteDisplay interface is bound to Models.SiteModel and any object created using ISiteDisplay must implement the both the private and protected members :( .

"Interfaces inherit even the private and protected members of a base class. This means that when you create an interface that extends a class with private or protected members, that interface type can only be implemented by that class or a subclass of it".

You can read more here - https://www.typescriptlang.org/docs/handbook/interfaces.html#interfaces-extending-classes

So, anytime we want to create a new object that inherit properties from it's parent, we also carry the burden of those children objects potentially breaking any time there's a change in the parent class.

Are we able to decorate our Model classes with Interfaces to give us greater flexibility? Or perhaps even remove the concept of Models altogether and just have the Controllers return data that match these Interfaces? e.g.

function createSite(): Promise<ISiteModel> {
	return Controllers.SiteController.create();
}

As for the newly addedfromJSON method - this could simply be moved to the respective Controller class.

Let me know you're thoughts. Hopefully I haven't thrown a spanner in the works :)

[ashneilson]

@josh-biddick Thanks for the feedback and raising this.

I'm wondering if we'd be best to get started using the API Client in it's current form and look to introduce improvements in the future?

Interfaces would help, I understand that, but they are a TypeScript concept and wouldn't be useful for consumers using JavaScript (many of our Apps are still JS only).

We could move the Model fromJSON() into the Controller, it would just require a bit of re-writing the PHP API Generation codebase.

I'll have a review and come back to you.

[ghost]

@ashneilson - Okay, that's fine. For now that will mean that I will need to create these Interfaces myself and centralise them somewhere until such time that we support them within the JS API Client. It's not so much a question of supporting Typescript as it is following SOLID priniciples that span all OOP languages (PHP included). I don't want us to get in a situation where we can't scale as the number of Models grow.

[ghost]

@ashneilson - Is there still work to be done on this or are you happy to merge this into master?

[ashneilson]

@josh-biddick I'd love to get this merged in so we can start implementing and testing in our Gen 4 Apps.

I'm wondering if any of your requested changes need to be made before we do this?

[ghost]

@ashneilson - Yup, I'm happy with that. As I mentioned, this will mean that for now, these interfaces/types will be defined in our Gen4 Apps using Typescript. Let's be aware that this does require code in two places should the underlying model structures change. I'll make a task in Wrike to revisit this for the future.