Types seem to be wrong - Githubissues

mrkev commented 1 year ago

Describe the bug

I'm fetching calendars. These are the keys for the data I'm seeing:

The types are nonetheless defined as:

export declare type CalendarProperties = {
    name: string;
    description: string;
    location: string;
    timezone: string;
    readOnly?: boolean;
    isPrimary?: boolean;
    jobStatusId?: string;
    metadata?: object;
    hexColor?: string;
};

export default class Calendar extends RestfulModel implements CalendarProperties {
    name: string;
    description: string;
    location: string;
    timezone: string;
    readOnly?: boolean;
    isPrimary?: boolean;
    jobStatusId?: string;
    metadata?: object;
    hexColor?: string;
    static collectionName: string;
    static attributes: Record<string, Attribute>;
    constructor(connection: NylasConnection, props?: CalendarProperties);
    save(params?: {} | SaveCallback, callback?: SaveCallback): Promise<this>;
    saveRequestBody(): Record<string, unknown>;
    getJobStatus(callback?: GetCallback): Promise<JobStatus>;
}

found in /node_modules/nylas/lib/models/calendar.d.ts.

Notably, properties that should be camelCase per the types are instead snake_case per the api. This makes the types unusable and is overall a usability hit to the SDK.

To Reproduce

Look at code.
Look at data actually coming from the api. \

Expected behavior Types match.

SDK Version:

"nylas": "^6.9.0",

Additional context N/A

mrashed-dev commented 1 year ago

Hey @mrkev thanks for opening this issue! How are you getting the keys in the first screenshot? Is it possible that you're either invoking toJSON or invoking a method that calls toJSON? Because our models override the behavour of toJSON by converting the keys to what the API expects (snake_case).

However when using the models within your application the attributes should all be camelCase. Calling calendar.isPrimary for example shouldn't throw an error and should return that value. Let me know if this isn't the case.

In the next major release of the Node SDK we are removing our implementation of toJSON so you should never see snake_case fields anywhere, but as of the current release the attributes should all be camelCase apart from calling toJSON.

birksy89 commented 1 year ago

@mrashed-dev I'm experiencing the same thing:

Calling:

const threads = await nylas.threads.list({ limit: 15, expanded: true });

Yields

const threads: Thread[]

A Thead Type looks like this - node_modules\nylas\lib\models\thread.d.ts:

The data I get is coming through as snake_case:

  "last_message_timestamp": 1693467986,
  "last_message_received_timestamp": 1693467986,
  "last_message_sent_timestamp": null,
  "first_message_timestamp": 1693467986,

Please can you let me know what I'm doing wrong - Thanks! 🙏

EDIT:

It might also be worth noting that when you look at the example code which is downloaded from the Nylas Walkthrough/Dashboard as a zip "nylas-send-and-read-emails-node-react", this too uses:

  <div className="time">
          {formatPreviewDate(
            new Date(Math.floor(thread.last_message_timestamp * 1000))
          )}
        </div>

mrashed-dev commented 1 year ago

@birksy89 Sorry for the delay on this response. How are you inspecting the key values of the object? If you are using toJSON like so:

const threads = await nylas.threads.list({ limit: 15, expanded: true });

threads.forEach(thread => {
  console.log(thread.toJSON());
});

Then it converts all the keys to snake_case as that's what the Nylas API expects for key casing.

If you were to just call console.log(thread) instead, you would see that the values are camelCased instead of snake_case'd. This is also why you see us call the fields using snakecase in our react example, because the backend component would serialize the thread object, effectively converting it to snake case.

Let me know if this is relevant to your inquiry or if I missed the mark here!

birksy89 commented 1 year ago

@mrashed-dev thank you for replying. I don't want to sound ungrateful, but have you tried to replicate this yourself?

I'm not using toJSON anywhere. I'm following examples in the code provided by Nylas.

I think this is pretty quick and easy to reconstruct. And again, I'd refer you to the example which you can download as a zip during onboarding. I don't think that uses toJSON either.

Sorry if I sound terse, I'm just on my phone as it's late here, and don't want to miss the window when your online 😄

Thanks again!

mrashed-dev commented 1 year ago

@birksy89 no problem at all! I did construct it and that's where I derived my answer from. This was the code I used:

const Nylas = require('./lib/nylas.js');

Nylas.config({
  clientId: 'MY_CLIENT_ID',
  clientSecret: 'MY_SECRET',
});

const nylas = Nylas.with('MY_ACCESS_TOKEN');

const threads = nylas.threads
  .list({ limit: 15, expanded: true })
  .then(threads => {
    threads.forEach(thread => {
      console.log(thread.toJSON());
      console.log(thread);
    });
  });

For each member of the loop, the two console.log statements gave me two different outputs:

for the .toJSON() call I got (similar to yours):

  last_message_timestamp: 1694631162,
  last_message_received_timestamp: 1694631162,
  last_message_sent_timestamp: null,
  first_message_timestamp: 1694631162,

and in the plain console.log(threads) I got:

  lastMessageTimestamp: 2023-09-13T18:52:42.000Z,
  lastMessageReceivedTimestamp: 2023-09-13T18:52:42.000Z,
  firstMessageTimestamp: 2023-09-13T18:52:42.000Z,

Note that in the second example how it's camelCase and the variable is a Date as opposed to an epoch timestamp.

Now mind you, you don't need to call toJSON() directly yourself to trigger it. toJSON() is a built-in javascript function. Some libraries will call toJSON as part of a serialization call or something, hence triggering the conversion to snake_case.

Can you provide me more context on how your using your "threads" variable after you fetch the data from our API? If you can share the function code snippet that would give me more clues as to what is going on :). Thanks!

birksy89 commented 12 months ago

You were right, @mrashed-dev!

Now mind you, you don't need to call toJSON() directly yourself to trigger it. toJSON() is a built-in javascript function. Some libraries will call toJSON as part of a serialization call or something, hence triggering the conversion to snake_case.

I think the issue stems from my use of tRPC and/or SuperJSON: https://trpc.io/docs/server/data-transformers

I've now installed a package called humps: https://www.npmjs.com/package/humps

readEmails: nylasClientProcedure.query(async ({ ctx }) => {
    const threads = await ctx.nylas.threads.list({ limit: 15, expanded: true });

    const newThreads = threads.map((thread) => {
      return humps.camelizeKeys(thread.toJSON()) as Thread;
    });

    return newThreads;
  }),

And when looking on the client side, this has the data as camelCase.

The Dates are messed up, but it's a bit of progress.

I'm not sure what the best approach is here - I've worked with 100s of APIs, and this is the first time I've come across this 🤔

Thanks for taking the time to help out here - Any further advice would be greatly appreciated

birksy89 commented 12 months ago

Although, I'm still confused about how this works for the Webhook side of things - As I'm pretty sure I'm getting errors there too.

"delta" is hitting my API endpoint with information like this:

{{
      date: 1693495014,
      object: 'message',
      type: 'message.created',
      object_data: {
        namespace_id: '4fbm8e1atg5pslxk6o4n2fro7',
        account_id: '4fbm8e1atg5pslxk6o4n2fro7',
        object: 'message',
        attributes: {
          thread_id: '35io4bu0lpmmp13z41bj31esi',
          received_date: 1693495004
        },
        id: '826okodim1qsunhzdt2cyu4ou',
        metadata: null
      }
    }
    message.created
    object_data {
      namespace_id: '4fbm8e1atg5pslxk6o4n2fro7',
      account_id: '4fbm8e1atg5pslxk6o4n2fro7',
      object: 'message',
      attributes: { thread_id: '35io4bu0lpmmp13z41bj31esi', received_date: 1693495004 },
      id: '826okodim1qsunhzdt2cyu4ou',
      metadata: null
    }}

But the types are like this:

object_data vs objectData

And then nested:

birksy89 commented 12 months ago

I can see they JSON is coming from the API, but it doesn't appear to be calling a TOJSON() function anywhere.

mrashed-dev commented 12 months ago

@birksy89 In the use-cases example the frontend code you are seeing does not contact the Nylas API directly but is communicating with the backend use-cases app. The backend use-cases app is running node + express and is handling the communication with the Nylas API and serves endpoints that the frontend can query to get the data from the Nylas API. The reason why it is returned as snake_case takes a bit of digging but:

We use express.js and we call res.json which,
Calls an internal function called stringify which,
Calls the builtin function JSON.stringify which,
Uses the object's toJSON() function which
Is ultimately overridden by us, who convert the object to snake_case 😅

I think this is the root of all the issues that you are seeing, and it's indeed the dangers of overriding an internal function such as toJSON(). Unfortunately while we can't fix this in the current version as it's a breaking change, we have removed this behaviour in the upcoming major version.

I am not sure how you are parsing the delta but if you're using a third party application it may very well be using some sort of JSON transformation which is triggering .toJSON().

birksy89 commented 12 months ago

Thank you so much for the thorough response, @mrashed-dev!

I have been looking at switching over to v3 - Do you think it's worthwhile doing in light of what we've discovered here? I'm not sure I'd want to invest much more time getting this to work neatly, only to rip it out later.

Webhook Deltas

I think what I'm getting back, matches what's expected by the documentation: https://developer.nylas.com/docs/developer-guide/webhooks/available-webhooks/#message-webhooks

You can see the data has properties such as account_id

But, unless I'm looking in the wrong place, these seems to be mismatched to the Typescript types

Located here: node_modules\nylas\lib\models\webhook-notification.d.ts

ObjectAttributes vs attributes

Line 99-100 in the screenshot above

This also really threw me off, as the "static" doesn't appear when using intelisense.

Thanks again for sticking with me on this 😅

Any final thoughts would be much appreciated 🙏

mrashed-dev commented 11 months ago

Hey! Sorry for the delay @birksy89.

Regarding Deltas

The typing looks right to me -- I think there's a piece missing in your screenshots.

WebhookNotfiicationProperties: Represents the entire object we receive from the API
WebhookDeltaProperties: Represents a single element in the "deltas" array response. Which includes keys like date, object, type, and object_data (The green line on the right photo).
WebhookObjectDataProperties: Represents the object_data object (the red line in the second photo) with the keys namespace_id, account_id, etc.

Regarding objectAttributes, there's a clash in the names (as you saw) between the API and the SDK. Each SDK model has an attributes field to handle serialization and deserialization. To work around that, we had to unfortunately rename the attributes field from the delta to objectAttributes.

I hope that clarifies it!

Regarding API v3

I definitely think that the SDK experience in the upcoming version is much better and helps fix many of the issues that you and many others have given us feedback on. The way we handle serialization and deserialization being one of them. Currently we have a beta of the Node SDK out for API v3 but at the moment we do not have support for messages, drafts, and threads. Support for that will likely come sometime next month. Does that timeline work with you?

birksy89 commented 11 months ago

Hey @mrashed-dev no problem - Thanks for replying.

I think it's just down to the casing again, like how here it's accountId https://github.com/nylas/nylas-nodejs/blob/c4a54f56d15200dbc0d1c87d8cbe8bc812273054/src/models/webhook-notification.ts#L240

But the API returns account_id

There's no parsing or "toJSON" performed here, this is me just parsing the webhook and saying the "type" is what I've found which matches the shape of the data.

API v3

I think we'll be moving to this soon - We do need messages and threads, but that timeline doesn't seem too far out so I'm sure we can wait.

Thanks for all your help on this!

mrashed-dev commented 9 months ago

@birksy89 sorry for the long delay in responding. WebhookObjectDataProperties, and any *Properties type isn't meant to be used directly, but instead the class is. WebhookObjectData should be deserialized properly when a JSON object gets sent to your application listening for new events. Here's how it's meant to be used:

https://github.com/nylas/nylas-nodejs/blob/c4a54f56d15200dbc0d1c87d8cbe8bc812273054/src/services/tunnel.ts#L99

There is where the conversion of snake_case to camelCase occurs.

The next version of the SDK now supports messaging, feel free to poke around here! https://github.com/nylas/nylas-nodejs/tree/v7.0.0-beta

birksy89 commented 9 months ago

No problem - Thanks for taking the time to respond 😄

We're using:

"nylas": "^7.0.0-beta.3",

In our application at the moment, and it's been a much smoother dev exp.

Thanks again for all your help and time on this

mrashed-dev commented 9 months ago

@birksy89 Love to hear it!! If you have any feedback please let us know :) I'll close this ticket for now. Thanks again for your patience!

nylas / nylas-nodejs

Types seem to be wrong #449

EDIT:

Webhook Deltas

ObjectAttributes vs attributes

API v3