[63533] Improve Node SDK typing#274
Conversation
|
This pull request has been linked to Shortcut Story #63533: Fix Node SDK Typing. |
|
|
||
| deleteRequestQueryString(params: { [key: string]: any } = {}) { | ||
| deleteRequestQueryString( | ||
| params: { [key: string]: any } = {} |
There was a problem hiding this comment.
@mrashed-dev for such instances like { [key: string]: any } - I think it might be better to opt to use the Record<keyType, valueType> built-in type.
For example params: Recod<string,any>.
There was a problem hiding this comment.
Separately, is there no way to avoid the use of any? It seems to defeat the purpose of typing :'(
If we do have to use any, could maybe unknown be of better use? (see article for more info)
There was a problem hiding this comment.
@AaronDDM thanks for bringing Record and unknown to my attention! Let me play with it and see how it works with the different methods we have.
There was a problem hiding this comment.
@AaronDDM I've updated the PR to include both changes!
| } | ||
|
|
||
| fromJSON(json?: any): this { | ||
| fromJSON(json: { [key: string]: any }): any { |
There was a problem hiding this comment.
Previously we returned this, but changing it to any would break typing for chaining, would it not?
e.g.
model.fromJSON({ ... }).toJSON()
^ typing would be broken and unable determine if `toJSON()` is validThere was a problem hiding this comment.
@AaronDDM good catch! Not sure why I reverted it. In any case, it's back in now.
| @@ -78,7 +95,7 @@ describe('Connect', () => { | |||
| email_address: exchangeEmail, | |||
There was a problem hiding this comment.
Some places we have emailAddress and some places email_address, is it possible to have this consistent throughout?
There was a problem hiding this comment.
This assertion (toHaveBeenCalledWith) checks the outgoing POST call. In the TS code we use emailAddress (camelCase) as the property name but when it gets serialized to JSON it becomes email_address (snake_case) to conform with what the API expects.
| fileIds(): (string | undefined)[] { | ||
| return this.files ? this.files.map(file => file.id) : []; | ||
| } |
There was a problem hiding this comment.
@mrashed-dev should this really be an array of string | undefined or can we filter out undefined ids? Not sure if there is a requirement to get the same # of array elements even if we don't have valid IDs.
| fileIds(): (string | undefined)[] { | |
| return this.files ? this.files.map(file => file.id) : []; | |
| } | |
| fileIds(): (string | undefined)[] { | |
| return this.files ? this.files.filter(file => file.id).map(file => file.id) : []; | |
| } |
There was a problem hiding this comment.
@AaronDDM I think that it will always return string | undefined because this.files is not guaranteed to be populated, which I think means it yields an undefined array type.
- used unknown instead of any when possible - used this instead of any when possible - used types instead of interfaces to allow for stronger typing
3 participants
Description
This PR is to address the Node SDK typing issues our customers have been experiencing. The biggest change we've made here is that all of the SDK methods that are meant to be used by the user for Nylas operations all have a non-any return type when possible.
In addition to this, the PR improves on the previous work of the refactor to ensure that we are utilizing the use of models, interfaces, and proper deserialization. With this, our support for Calendar availability has improved with the introduction of classes and types for API objects like free-busy and availability. Our support for Native Authentication and Virtual Calendars have also seen improvements as we have models to represent these objects and introduced enums to provide the user an easier time with providing things like providers and scopes without needing to refer to the API docs to see what scopes/providers are supported and to reduce user typing errors.
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.