Skip to content

[66696] Improve deserialization and object creation#271

Merged
mrashed-dev merged 22 commits into
mostafarashed/ch66699/release-node-sdk-v6-0-0from
mostafarashed/ch66696/improve-deserialization
Sep 9, 2021
Merged

[66696] Improve deserialization and object creation#271
mrashed-dev merged 22 commits into
mostafarashed/ch66699/release-node-sdk-v6-0-0from
mostafarashed/ch66696/improve-deserialization

Conversation

@mrashed-dev
Copy link
Copy Markdown
Contributor

@mrashed-dev mrashed-dev commented Sep 2, 2021

This PR is a big one, it's aimed at two things -- improving how we deserialize objects while improving how users can create objects using the SDK.

Now, each Model type has an accompanying interface that represents an object of parameters that the Model will use, as well as the proper strictness dictated by what values will always be present in any scenario (GET, POST, PUT). In the constructor of each model, we now specify that we can take in an optional props property of the Model interface's type. This helps to hint the user as to what properties they must pass in during the initialization of the object. This also helps us with the strictness of the typing in each model as now we properly outline what properties are needed at a bare minimum.

This props method property is left as optional because it allows some flexibility to the SDK user to just instantiate a type of object then either:

  • performing [model].fromJSON()
  • or, setting the property values individually via [model].[property] = value
    If taking this route, we initialize all the required types to a default value in order not to upset Typescript's strictness requirements.

With this, we also deprecate the Nylas.[model].build() method as it provides a way for the user to bypass the constructor typing requirements and pass in whatever object they want. It also does not provide any type hinting to the user when trying to initialize the object, rendering this work useless. Here's an example of the before and after:

const Nylas = require('nylas');
Nylas.config({clientId: 'clientId', clientSecret: 'clientSecret'});
const nylas = Nylas.with('access_token');

// before, user can put whatever they want
const event = nylas.events.build({foo: "bar"});

// now, this way the user has to abide by EventProperties or else face a warning from Typescript

// throws warning:
const event = new Event(nylas, {foo: "bar"});

// does not throw warning
const event = new Event(nylas, {calendarId: "id", when: {startTime: 1234, endTime: 4321}});

// also does not throw warning, inits and object that has default values
const event = new Event(nylas);

License

I confirm that this contribution is made under the terms of the MIT license and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Created two new "base" classes called `model` and `model-collection`. These two classes provide a way for creating models that are not the typical Nylas API models (Event, Calendar, etc.) but will still require similar functionality. Also not all models are "RESTful", `Contact.Email` for example does not.
We are moving on from only supporting instantiating objects from JSON to instantiating objects from properties formatted via camelCase
A followup to the previous commit. Now we can pass in a properties object to properly instantiate an object. The typing is also now fixed as now required properties are no longer optional. We have set them to a default value to allow the users to choose how to create an object - either via passing in a properties object or by setting each field manually.
Setting a default property value will always set that default value after the constructor is executed, meaning if we deserialize in the constructor, the desearilized value will be reset. We tackle this by setting default values for strict properties if no `props` object is passed in.
This method makes it impossible to provide hinting to the user when they want to create a new object. A user should be using `new Model()` instead of using `Nylas.model.build()`.
@shortcut-integration
Copy link
Copy Markdown

This pull request has been linked to Clubhouse Story #66696: Improve Deserialization.

@mrashed-dev mrashed-dev changed the base branch from main to mostafarashed/ch66695/refactor-restfulmodel September 2, 2021 01:00
Comment thread src/models/calendar.ts Outdated

constructor(connection: NylasConnection, props?: CalendarProperties) {
super(connection, props);
if(!props) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can do it this way, or we can make CalendarProperties less strict allowing users to pass in as many or as little initial properties and basically do this.name = props?.name ? props.name : ""; for every required property. (This would be done to all similar models).

Comment thread src/models/calendar.ts Outdated

export default class Calendar extends RestfulModel
implements CalendarProperties {
name!: string;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added bang (!) as a modifier because we guarantee to set these values somehow -- either via the props that are sent in (which contain the strict properties) or if no props are sent in via setting default values.

@mrashed-dev mrashed-dev marked this pull request as ready for review September 2, 2021 22:22
@mrashed-dev
Copy link
Copy Markdown
Contributor Author

Ignore merge conflicts for now please! The merge resolution will occur after #267 is merged. Jest is reporting all tests passing and linter + prettier was run on this project.

Copy link
Copy Markdown

@philrenaud philrenaud left a comment

Choose a reason for hiding this comment

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

Good explanation; this is a nice change!

Base automatically changed from mostafarashed/ch66695/refactor-restfulmodel to mostafarashed/ch66699/release-node-sdk-v6-0-0 September 3, 2021 14:15
Comment thread src/models/contact.ts Outdated

constructor(props?: IMAddressProperties) {
super(props);
if (!props) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we just initialize the props with '' instead of having this check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, if we initialize props with anything other than an object of type IMAddressProperties (object with type and imAddress set) because of the strictness on those properties. The only way to do it like you're suggesting is to do something like constructor(props: IMAddressProperties = {type: '', imAddress: ''}) {

Alternatively, we can make all of these interfaces less strict allowing users to pass in as many or as little initial properties and in the do something like this.type = props?.type ? props.type : ""; for every required property.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have re-worked the props to match what we talked about earlier. The properties now have default values without any extra checks.

Have to call `this.initAttributes(props);` instead of having it in the super class due to Typescript design: https://stackoverflow.com/questions/43595943/why-are-derived-class-property-values-not-seen-in-the-base-class-constructor
Instead of having just a type, this will allow us to move `parseContacts` into the class
Comment thread src/models/event.ts
this.calendarId = '';
this.when = new When();
}
this.initAttributes(props);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason why all the classes have this initAttributes call instead of having it in the super class constructor (as it was earlier) is due to how TypeScript is designed in regards to how a child class is initialized. More info can be found here (thanks to @ozsivanov for finding this!): https://stackoverflow.com/questions/43595943/why-are-derived-class-property-values-not-seen-in-the-base-class-constructor

Remove snake_case support and create a new model to better support free-busy
@mrashed-dev mrashed-dev merged commit c52440a into mostafarashed/ch66699/release-node-sdk-v6-0-0 Sep 9, 2021
@mrashed-dev mrashed-dev deleted the mostafarashed/ch66696/improve-deserialization branch September 9, 2021 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants