Skip to content

Conversation

@bajtos
Copy link
Member

@bajtos bajtos commented Sep 5, 2016

Change the format of Date "returns" arguments from the output of .toString(), which produces values in local timezone, to the output of .toJSON(), which produces values in GMT.

Before:

{
  "$type": "date",
  "$data": "Thu Jan 01 1970 01:00:00 GMT+0100 (CET)"
}

Now:

{
  "$type": "date",
  "$data": "1970-01-01T00:00:00.000Z"
}

Connect to strongloop/loopback#1990

@ritch please review
@gunjpan @richardpringle @deepakrkris PTAL too

Note: in the issue strongloop/loopback#1990, I am proposing a more radical change. Unfortunately, it also requires more time which we don't have right now.

@bajtos bajtos added the #review label Sep 5, 2016
@bajtos bajtos force-pushed the fix/date-serialization branch 2 times, most recently from 53773d0 to b21a02a Compare September 5, 2016 08:54
@ritch
Copy link
Member

ritch commented Sep 6, 2016

Are there complaints / issues with the existing support? This could break applications in a very subtle way that will be hard to detect.

That being said toJSON is the ideal format

@bajtos
Copy link
Member Author

bajtos commented Sep 7, 2016

Are there complaints / issues with the existing support?

Not really. We discovered this issue while working on iOS code generator and also while I was working on the rest-coercion test suite.

This could break applications in a very subtle way that will be hard to detect.

TBH, I don't think there are many users relying on this feature. It requires a remote methods that has a "returns" argument with type "date" and there are no such methods in LoopBack. Users would have to write their own custom method.

I also believe this change is not as bad is it may look at the first sight. Both strings Thu Jan 01 1970 01:00:00 GMT+0100 (CET) and 1970-01-01T00:00:00.000Z are a different representation of the same point in time.

I think there are only two things that can happen:

  • Clients capable of parsing both date formats (e.g. new Date(str) in JavaScript)` will not notice any difference.
  • Clients that understand the first format only should start failing (complaining about invalid date string).

It is also possible to have a client that silently ignores parsing errors and returns arbitrary date value in such case, but I would argue that's a bug in the client implementation.

@ritch What's your verdict? Merge or close?

@richardpringle
Copy link
Contributor

I think the change makes sense. @ritch also makes a good point that it could disrupt clients that aren't parsing the string "properly" but I don't think that we should cater to them.

I think that as long as the change is highlighted that it could be a breaking change in the release notes it should be fine.

@ritch
Copy link
Member

ritch commented Sep 12, 2016

Merge it.

@bajtos bajtos force-pushed the fix/date-serialization branch from b21a02a to cc6e0a0 Compare September 13, 2016 07:15
Change the format of Date "returns" arguments from the output
of `.toString()`, which produces values in local timezone, to the output
of `.toJSON()`, which produces values in GMT.

Before:

    {
      "$type": "date",
      "$data": "Thu Jan 01 1970 01:00:00 GMT+0100 (CET)"
    }

Now:

    {
      "$type": "date",
      "$data": "1970-01-01T00:00:00.000Z"
    }
@bajtos bajtos force-pushed the fix/date-serialization branch from cc6e0a0 to 614b8b6 Compare September 13, 2016 07:22
@bajtos
Copy link
Member Author

bajtos commented Sep 13, 2016

@slnode test please

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.

4 participants