bug: dates are wrong type in Jest

[ffMathy]

Dates are typed as string, whereas the real type seems to be { value: string }.

[gismya]

Which dates on which entity types? We're quite extensively working with dates at the moment and haven't seen that issue.

[ffMathy]

The start_date and end_date of a Task, it seems.

[gismya]

Ah, right. I forgot that all our queries are done using the option { decodeDatesAsIso: true }, which uses the upcoming default behaviour of being strings. The types are, as you say, incorrect unless you use that option. They currently come back as Moment object otherwise. We looked a bit at setting the type depending on the options used, but it felt messy (And is one of the reasons we're moving to a single return type as we're discussing in #71).

[ffMathy]

Aha. I think it's OK for us to use that option as well. Thank you for clearing that up!

Closing in favor of #71 then.

[ffMathy]

Wait, we just tried this option. It doesn't work. I suppose it means that the option is not yet out until #71 is done? Will re-open this.

What alternatives to we have for now?

[gismya]

That option has been available since quite a while.

The option is on a query/mutation level, the second parameter in any query or mutation takes an options object.

[ffMathy]

Ah interesting.

I think with that, we'd be afraid to forget to specify it. Can we make it support setting query-level default options in the constructor of the session?

[gismya]

There's already a way to globally add additionalHeaders, the other big thing to use the options for (that makes sense to have globally). Hence, I don't see an issue except that it's planned for removal in 2.0, and might just be best to get to a 2.0 release instead. Create an issue in the JavaScript API repo, and we'll see if we can decide on how to handle it. Meanwhile, I'll try to get a finalization of 2.0 approved for the short-term backlog.

[ffMathy]

Great. Thank you for that update.

I agree. Maybe this issue can serve as that?

[gismya]

We actually took a decision about the 2.0 release earlier today, so your PR had great timing. We tried getting the 2.0 out this week or the next, but we found internal dependencies that would break with the planned 2.0 changes. They were already scheduled to be removed in the coming month, so it made no sense to rebuild them. We also don't want to release the JavaScript API to a state where we wouldn't be using the latest version. We'll get your PR reviewed and released ASAP.

[ffMathy]

Awesome stuff! Great news, and thanks for keeping us posted!

[ffMathy]

Can we reopen this @gismya? We just tried setting that option, and it doesn't work. The strong-typing types it as string but the real value seems to be { value: string } still.

[ffMathy]

Note that we tried setting it in the constructor, so perhaps it's my fix that doesn't work. Not sure.

[gismya]

Looks like it should work. Make sure you're running the latest version (1.8.3) and initializing the constructor settings correctly
e.g:

    const decodeDatesAsIsoSession = new Session(
      credentials.serverUrl,
      credentials.apiUser,
      credentials.apiKey,
      {
        decodeDatesAsIso: true,
      },
    );

If you still have issues, send a code sample I can tests that has the issue and I'll try to find any bugs.

[ffMathy]

It's so weird. That's exactly what we're doing. And we have that latest version.

I'll see if I can make a better repro.

[ffMathy]

@gismya it is indeed still happening. The type of (for instance) end_date of a Task is just string, but the real runtime type is { value: string }.

Repro:

const session = new Session(
  ftrackServerUrl,
  ftrackApiUser,
  ftrackApiKey,
  { decodeDatesAsIso: true },
);
await session.initializing;

const project = await session.ensure<Project>('Project', {
  end_date: '2023-12-31T00:00:00',
  name: "some-random-project-name",
});
//here, project.end_date is of type string according to TypeScript.
//but as you can see in the error below, the runtime type is different.
expect(project.end_date).toBe('2023-12-31T00:00:00');

Error:

Error: expect(received).toBe(expected) // Object.is equality

Expected: "2023-12-31T00:00:00"
Received: {"__type__": "datetime", "value": "2023-12-31T00:00:00"}

The above error is for the Project type, but it doesn't matter what type we use.

[ffMathy]

Based on the above, can we re-open as well?

[ffMathy]

Thanks. Also, I'm not sure if this is a bug in the typings or the issue should be moved to the API client project.

[gismya]

Sounds like an API client error, but I'll look into it.

[gismya]

That's not a working reproduction example. Are you using mocking in your tests, or is that running towards an instance? Because the returned value looks incorrect, there should be no type key in the object, it should be __type__

[ffMathy]

No mocking involved - it's an integration test running directly towards a real Ftrack instance, running version 4.12.

However, the types were generated waaaaaay back. Just right after the TS generator was published, and back then, we were using Ftrack 4.10 (so almost 2 year old version). Could this have been the cause?

But then, what I don't get is that the type is wrong at runtime.

When I did the repro, it was just that example I gave you. No modifications. It still fails, towards a real Ftrack instance.

Very strange that you're not getting the same results!

[gismya]

It seems like the result you get from the server is incorrect, which is super weird. This is an on prem instance?

Also, I saw that the GitHub formatting incorrectly picked up the keys I mentioned in the last post: the issue is that the __type__ key is getting to you as type.

But the issue isn't with the type or the API client from what I can tell.

[ffMathy]

Interesting. Yes, it's an on-prem instance.

Hmmm is that what causes the schema type generator to generate wrong types then, or?

[gismya]

It would cause the API client to not convert it to a string (That the schema says it is). Can you try to verify the data coming from the server somehow, it could also be the test runner, the pasting to github or something else that removes the __? Try running it through the browser console on the actual instance (In the projects, reports or my task tabs) using

const session = ftrackSpark.getSession();
await ftrackSpark.getSession().query('select start_date from Project limit 1')

and inspect the response in the network tab. It should look like

[
    {
        "action": "query",
        "data": [
            {
                "__entity_type__": "Project",
                "id": "17496542-b5b5-4107-9624-0c8180df9969",
                "context_type": "show",
                "start_date": {
                    "__type__": "datetime",
                    "value": "2023-08-18T07:06:33"
                }
            }
        ],
        "metadata": {
            "next": {
                "offset": 1
            }
        }
    }
]

[ffMathy]

It was indeed GitHub ruining the formatting. I changed from a quote style block into a code block now. I could also see when I edited the comment that the code was indeed __type__.

I saved the modification to the comment now, so it reflects the latest.

Does that change anything?

I'll try the given line in the console soon.

[ffMathy]

Running the command on my trial cloud account gives me this result:

{"action":"query","data":[{"__entity_type__":"Project","id":"33b1e354-8b3d-11eb-8e99-c2ffbce28b68","context_type":"show","start_date":"2023-10-02T00:00:00.000Z"}],"metadata":{"next":{"offset":1}}}

[ffMathy]

The command on our on-prem cluster yields a similar result:

{"action":"query","data":[{"context_type":"show","__entity_type__":"Project","id":"000877aa-6e5e-11ee-a4cd-a260f9cca335","start_date":"2020-01-01T00:00:00.000Z"}],"metadata":{"next":{"offset":1}}}

[ffMathy]

Does it matter if it is run from Node or web perhaps? We are running it from Node.

[gismya]

Still can't find anything wrong either through the browser or node ,on any instance I have or create. I'll close this until I get a reproducible example. From what I see now, my best guess is that the test runner is doing something weird with the value before comparing. Make sure the value you get from a query really is not a string.

[ffMathy]

Still can't find anything wrong either through the browser or node ,on any instance I have or create. I'll close this until I get a reproducible example. From what I see now, my best guess is that the test runner is doing something weird with the value before comparing. Make sure the value you get from a query really is not a string.

That's the weird part. Our error also occurs in production.

I will try to see if one of our other devs can repro, and then get back to you.

[ffMathy]

I found the cause @gismya! But not sure how to solve it.

I think it has to do with differences in the value of serverInformation.is_timezone_support_enabled on some of our environments!

But not sure how that is triggered. Is it a setting somewhere in Ftrack?

[gismya]

I don't know how the server settings are set up for on prem. Send an email to support for further help with that.

[ffMathy]

Alright, will ping them.

However, actually, some further thoughts... Should that setting even affect the decoding? Why is that check actually there? In my naive viewpoint, decoding of times as strings instead of dates should be unrelated to whether timezone support is enabled. It should be supported either way, right?

[gismya]

It should not break, and I don't know why it does, but since it's a deprecated setting we probably won't make any investigations on it or changes regarding how they are handled in the API client.

[ffMathy]

Yeah makes sense. Any plans on dropping support for older Ftrack servers anytime soon?

[ffMathy]

Also, I wonder if it changes anything to run it via Jest.

[ffMathy]

Also, this line could be suspicious. Is it needed?

https://github.com/ftrackhq/ftrack-javascript/blob/39c5e9297156f2b74d3da0adddc291766942fb0a/source/session.ts#L373C1-L373C1

I understand the check for object, but why the check for the constructor of the object? Maybe that one is somehow different in Jest or whatever?

[gismya]

Because JavaScript is weird.

typeof {} === "object" //true
Makes sense
typeof [] === "object" // true
A bit of a stretch, but sure
typeof null === "object" //true
What?
typeof (() => {}) === "object" // false
But.. A function is an object?

But I think you're right that something in the decoding function is behaving differently in your Node environment. Try making a custom build of the API client with a bunch of logs there to try to find the culprit. After we find what behaves differently we can find an alternate solution to the decoding function.

[ffMathy]

Awesome, good idea! Will do, and get back.

[ffMathy]

I have some interesting findings. I attached a debugger, and landed here after the ensure<Project> call:

Then, at that point, I tried evaluating some things in the debug console.

This indeed confirms my suspicion! Even though data.constructor seems to return Object, data.constructor === Object evaluates to false!

This is from my Jest environment.

[ffMathy]

Oh, and decodeDatesAsIso was indeed true at the time.

[ffMathy]

If I change the function into this, it works:

    decode(data, identityMap = {}, decodeDatesAsIso = false) {
      if (Array.isArray(data)) {
        return this._decodeArray(data, identityMap, decodeDatesAsIso);
      }
      if (!!data && typeof data === "object") {
        if (data.__entity_type__) {
          return this._mergeEntity(data, identityMap, decodeDatesAsIso);
        }
        if (data.__type__ === "datetime" && decodeDatesAsIso) {
          return this._decodeDateTimeAsIso(data);
        } else if (data.__type__ === "datetime") {
          return this._decodeDateTimeAsMoment(data);
        }
        return this._decodePlainObject(data, identityMap, decodeDatesAsIso);
      }
      return data;
    }

Although we still don't understand the root cause (my suspicion would be that Jest does some default import mocking where it replaces the Object constructor or whatever), would this code be problematic?

The way I see it, it would still do the same, and handle all the cases you described above. Will make a PR for this, just in case.

[ffMathy]

Also, I did a separate Jest test with these checks:

Object === {}.constructor //evaluates to true
Object === Object.create(null).constructor //evaluates to false
Object === Object.create({}).constructor //evaluates to true

That seems to be pretty correct to me. So I don't get why it's different with the Ftrack API.

By the way, the file that ends up getting imported is ftrack-javascript-api.umd.cjs, because I am using CommonJS. Could it be some stuff that the bundler does with the CommonJS bundle of Ftrack somehow?

Also, is the behavior of those 3 lines intentional? Not sure how the Ftrack API creates its objects. But may it proves that this check is not the best, and should be excluded altogether or changed?

[ffMathy]

And one more thing... I feel like we have enough information now to re-open it 🙏

Perhaps even move it to ftrackhq/ftrack-javascript.

[gismya]

Good find. You can open this in the ftrack-javascript repository, with a short summary and then you can link to this issue for the details. With this we definitely have enough information to solve this. But that's optional, we can solve it without that if you don't want to create it.

[ffMathy]

There's a move button in github's UI though, and that'd move the full discussion too. I think that makes most sense, but only you can do that.

If you still think I should make a new issue, I will 🙏

[gismya]

Did not know about that. Thank you!

[ffMathy]

You're very welcome, and thanks for merging! 😍