Skip to content

feat: Support encoding and decoding of non-moment date objects #78

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Feb 22, 2023
Merged

feat: Support encoding and decoding of non-moment date objects #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
163d34e
feat: support encoding non-moment date objects
jimmycallin Feb 14, 2023
99c1c8e
add tests for convert_to_iso_string
jimmycallin Feb 19, 2023
4604cdc
added docstring
jimmycallin Feb 19, 2023
e401e39
add additional encoding tests
jimmycallin Feb 20, 2023
001eb83
support non-moment decoding
jimmycallin Feb 20, 2023
26351d4
non-UTC timezone for tests in CI
jimmycallin Feb 20, 2023
83ed053
Merge branch 'main' into support-non-moment-encoding
jimmycallin Feb 21, 2023
0394d8b
consistent camel casing
jimmycallin Feb 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ on:
jobs:
build:
runs-on: ubuntu-latest
env:
TZ: Europe/Stockholm
steps:
- uses: actions/checkout@v3
- name: Using Node.js from .nvmrc
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"@rollup/plugin-commonjs": "^24.0.1",
"@types/uuid": "^9.0.0",
"cross-fetch": "^3.1.5",
"dayjs": "^1.11.7",
"eslint": "^8.33.0",
"eslint-config-react-app": "^7.0.1",
"jsdom": "^21.1.0",
Expand Down
143 changes: 101 additions & 42 deletions source/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { SERVER_LOCATION_ID } from "./constant";

import normalizeString from "./util/normalize_string";
import { Data } from "./types";
import { convertToIsoString } from "./util/convert_to_iso_string";

const logger = loglevel.getLogger("ftrack_api");

Expand Down Expand Up @@ -95,18 +96,20 @@ export interface ResponseError {
error?: Data;
}

export interface MutatationOptions {
export interface MutationOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤓 🏅

pushToken?: string;
additionalHeaders?: Data;
decodeDatesAsIso?: boolean;
}

export interface QueryOptions {
abortController?: AbortController;
signal?: AbortSignal;
additionalHeaders?: Data;
decodeDatesAsIso?: boolean;
}

export interface CallOptions extends MutatationOptions, QueryOptions {}
export interface CallOptions extends MutationOptions, QueryOptions {}

/**
* ftrack API session
Expand Down Expand Up @@ -141,6 +144,7 @@ export class Session {
* @param {string} [options.apiEndpoint=/api] - API endpoint.
* @param {object} [options.headers] - Additional headers to send with the request
* @param {object} [options.strictApi] - Turn on strict API mode
* @param {object} options.decodeDatesAsIso - Decode dates as ISO strings instead of moment objects
*
* @constructs Session
*/
Expand Down Expand Up @@ -307,9 +311,7 @@ export class Session {
/**
* Return encoded *data* as JSON string.
*
* This will translate objects with type moment into string representation.
* If time zone support is enabled on the server the date
* will be sent as UTC, otherwise in local time.
* This will translate date, moment, and dayjs objects into ISO8601 string representation in UTC.
*
* @private
* @param {*} data The data to encode.
Expand All @@ -331,7 +333,8 @@ export class Session {
return out;
}

if (data && data._isAMomentObject) {
const date = convertToIsoString(data);
if (date) {
if (
this.serverInformation &&
this.serverInformation.is_timezone_support_enabled
Expand All @@ -340,15 +343,15 @@ export class Session {
// to timezone naive string.
return {
__type__: "datetime",
value: data.utc().format(ENCODE_DATETIME_FORMAT),
value: date,
};
}

// Ensure that the moment object is in local time zone and format
// to timezone naive string.
return {
__type__: "datetime",
value: data.local().format(ENCODE_DATETIME_FORMAT),
value: moment(date).local().format(ENCODE_DATETIME_FORMAT),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we target the v2 for these changes as well? I am not really sure how to test this for regressions, so feels safer to do this while also removing this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have intentionally separated code for this PR to be backwards compatible by making sure all moment objects are passed in the same manner as before, so I hope we can target this for a minor release. we can add additional tests for this if there are certain use cases you are unsure about.

Copy link
Contributor

Choose a reason for hiding this comment

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

I want to ensure that this path hasn't changed, without having to disable timezone support locally.

Can we add a test for:

const date = moment()
const convertedDate = convertToISOString(date)
const encodedDate = moment(date).local().format(ENCODE_DATETIME_FORMAT)

assert date.local().format(ENCODE_DATETIME_FORMAT) === encodedDate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please see new tests!

};
}

Expand Down Expand Up @@ -398,22 +401,54 @@ export class Session {
* @return {*} Decoded data
*/

private decode(data: any, identityMap: Data = {}): any {
private decode(
data: any,
identityMap: Data = {},
decodeDatesAsIso: boolean = false
): any {
if (Array.isArray(data)) {
return this._decodeArray(data, identityMap);
return this._decodeArray(data, identityMap, decodeDatesAsIso);
}
if (typeof data === "object" && data?.constructor === Object) {
if (data.__entity_type__) {
return this._mergeEntity(data, identityMap);
return this._mergeEntity(data, identityMap, decodeDatesAsIso);
}
if (data.__type__ === "datetime") {
return this._decodeDateTime(data);
if (data.__type__ === "datetime" && decodeDatesAsIso) {
return this._decodeDateTimeAsIso(data);
} else if (data.__type__ === "datetime") {
return this._decodeDateTimeAsMoment(data);
}
return this._decodePlainObject(data, identityMap);
return this._decodePlainObject(data, identityMap, decodeDatesAsIso);
}
return data;
}

/**
* Decode datetime *data* into ISO 8601 strings.
*
* Translate objects with __type__ equal to 'datetime' into moment
* datetime objects. If time zone support is enabled on the server the date
* will be assumed to be UTC and the moment will be in utc.
* @private
*/
private _decodeDateTimeAsIso(data: any) {
let dateValue = data.value;
if (
this.serverInformation &&
this.serverInformation.is_timezone_support_enabled
) {
// Server responds with timezone naive strings, add Z to indicate UTC.
// If the string somehow already contains a timezone offset, do not add Z.
if (!dateValue.endsWith("Z") && !dateValue.includes("+")) {
dateValue += "Z";
}
// Return date as moment object with UTC set to true.
return new Date(dateValue).toISOString();
}
// Server has no timezone support, return date in ISO format
return new Date(dateValue).toISOString();
}

/**
* Decode datetime *data* into moment objects.
*
Expand All @@ -422,7 +457,7 @@ export class Session {
* will be assumed to be UTC and the moment will be in utc.
* @private
*/
private _decodeDateTime(data: any) {
private _decodeDateTimeAsMoment(data: any) {
if (
this.serverInformation &&
this.serverInformation.is_timezone_support_enabled
Expand All @@ -439,9 +474,13 @@ export class Session {
* Return new object where all values have been decoded.
* @private
*/
private _decodePlainObject(object: Data, identityMap: Data) {
private _decodePlainObject(
object: Data,
identityMap: Data,
decodeDatesAsIso: boolean
) {
return Object.keys(object).reduce<Data>((previous, key) => {
previous[key] = this.decode(object[key], identityMap);
previous[key] = this.decode(object[key], identityMap, decodeDatesAsIso);
return previous;
}, {});
}
Expand All @@ -450,15 +489,25 @@ export class Session {
* Return new Array where all items have been decoded.
* @private
*/
private _decodeArray(collection: any[], identityMap: Data): any[] {
return collection.map((item) => this.decode(item, identityMap));
private _decodeArray(
collection: any[],
identityMap: Data,
decodeDatesAsIso: boolean
): any[] {
return collection.map((item) =>
this.decode(item, identityMap, decodeDatesAsIso)
);
}

/**
* Return merged *entity* using *identityMap*.
* @private
*/
private _mergeEntity(entity: Data, identityMap: Data) {
private _mergeEntity(
entity: Data,
identityMap: Data,
decodeDatesAsIso: boolean
) {
const identifier = this.getIdentifyingKey(entity);
if (!identifier) {
logger.warn("Identifier could not be determined for: ", identifier);
Expand All @@ -479,7 +528,11 @@ export class Session {

for (const key in entity) {
if (entity.hasOwnProperty(key)) {
mergedEntity[key] = this.decode(entity[key], identityMap);
mergedEntity[key] = this.decode(
entity[key],
identityMap,
decodeDatesAsIso
);
}
}
return mergedEntity;
Expand Down Expand Up @@ -511,6 +564,7 @@ export class Session {
* @param {AbortSignal} options.signal - Abort signal
* @param {string} options.pushToken - push token to associate with the request
* @param {object} options.headers - Additional headers to send with the request
* @param {string} options.decodeDatesAsIso - Return dates as ISO strings instead of moment objects
*
*/
call(
Expand All @@ -520,6 +574,7 @@ export class Session {
pushToken,
signal,
additionalHeaders = {},
decodeDatesAsIso = false,
}: CallOptions = {}
): Promise<Response<Data>[]> {
const url = `${this.serverUrl}${this.apiEndpoint}`;
Expand Down Expand Up @@ -574,7 +629,7 @@ export class Session {
})
.then((data) => {
if (this.initialized) {
return this.decode(data);
return this.decode(data, {}, decodeDatesAsIso);
}

return data;
Expand Down Expand Up @@ -657,10 +712,8 @@ export class Session {

if (value != null && typeof value.valueOf() === "string") {
value = `"${value}"`;
} else if (value && value._isAMomentObject) {
// Server does not store microsecond or timezone currently so
// need to strip from query.
value = moment(value).utc().format(ENCODE_DATETIME_FORMAT);
} else if (convertToIsoString(value)) {
value = convertToIsoString(value);
value = `"${value}"`;
}
return `${identifyingKey} is ${value}`;
Expand Down Expand Up @@ -734,6 +787,7 @@ export class Session {
* @param {object} options.abortController - Deprecated in favour of options.signal
* @param {object} options.signal - Abort signal user for aborting requests prematurely
* @param {object} options.headers - Additional headers to send with the request
* @param {object} options.decodeDatesAsIso - Decode dates as ISO strings instead of moment objects
* @return {Promise} Promise which will be resolved with an object
* containing action, data and metadata
*/
Expand Down Expand Up @@ -761,6 +815,7 @@ export class Session {
* @param {object} options.abortController - Deprecated in favour of options.signal
* @param {object} options.signal - Abort signal user for aborting requests prematurely
* @param {object} options.headers - Additional headers to send with the request
* @param {object} options.decodeDatesAsIso - Decode dates as ISO strings instead of moment objects
* @return {Promise} Promise which will be resolved with an object
* containing data and metadata
*/
Expand Down Expand Up @@ -805,17 +860,18 @@ export class Session {
* @param {Object} options
* @param {string} options.pushToken - push token to associate with the request
* @param {object} options.headers - Additional headers to send with the request
* @param {object} options.decodeDatesAsIso - Decode dates as ISO strings instead of moment objects
* @return {Promise} Promise which will be resolved with the response.
*/
create(entityType: string, data: Data, { pushToken }: CallOptions = {}) {
logger.debug("Create", entityType, data, pushToken);
create(entityType: string, data: Data, options: MutationOptions = {}) {
logger.debug("Create", entityType, data, options);

let request = this.call([operation.create(entityType, data)], {
pushToken,
}).then((responses) => {
const response = responses[0];
return response;
});
let request = this.call([operation.create(entityType, data)], options).then(
(responses) => {
const response = responses[0];
return response;
}
);

return request;
}
Expand All @@ -829,19 +885,21 @@ export class Session {
* @param {Object} options
* @param {string} options.pushToken - push token to associate with the request
* @param {object} options.headers - Additional headers to send with the request
* @param {object} options.decodeDatesAsIso - Decode dates as ISO strings instead of moment objects
* @return {Promise} Promise resolved with the response.
*/
update(
type: string,
keys: string[],
data: Data,
{ pushToken }: MutatationOptions = {}
options: MutationOptions = {}
) {
logger.debug("Update", type, keys, data, pushToken);
logger.debug("Update", type, keys, data, options);

const request = this.call([operation.update(type, keys, data)], {
pushToken,
}).then((responses) => {
const request = this.call(
[operation.update(type, keys, data)],
options
).then((responses) => {
const response = responses[0];
return response;
});
Expand All @@ -857,12 +915,13 @@ export class Session {
* @param {Object} options
* @param {string} options.pushToken - push token to associate with the request
* @param {object} options.headers - Additional headers to send with the request
* @param {object} options.decodeDatesAsIso - Decode dates as ISO strings instead of moment objects
* @return {Promise} Promise resolved with the response.
*/
delete(type: string, keys: string[], { pushToken }: MutatationOptions = {}) {
logger.debug("Delete", type, keys, pushToken);
delete(type: string, keys: string[], options: MutationOptions = {}) {
logger.debug("Delete", type, keys, options);

let request = this.call([operation.delete(type, keys)], { pushToken }).then(
let request = this.call([operation.delete(type, keys)], options).then(
(responses) => {
const response = responses[0];
return response;
Expand Down
34 changes: 34 additions & 0 deletions source/util/convert_to_iso_string.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* Checks if string is in ISO 6801 format.
*/
function isIsoDate(str: string) {
return /^([+-]?\d{4}(?!\d{2}\b))((-?)((0[1-9]|1[0-2])(\3([12]\d|0[1-9]|3[01]))?|W([0-4]\d|5[0-2])(-?[1-7])?|(00[1-9]|0[1-9]\d|[12]\d{2}|3([0-5]\d|6[1-6])))([T\s]((([01]\d|2[0-3])((:?)[0-5]\d)?|24:?00)([.,]\d+(?!:))?)?(\17[0-5]\d([.,]\d+)?)?([zZ]|([+-])([01]\d|2[0-3]):?([0-5]\d)?)?)?)?$/.test(
str
);
}

/**
* Converts a string or date object to ISO 6801 compatible string.
* Supports converting regular date objects, or any object that has toISOString() method such as moment or dayjs.
*
* @param data - string or date object
* @returns ISO 6801 compatible string, or null if invalid date
*/
export function convertToIsoString(data: string | Date) {
if (
data &&
// if this is a date object of type moment or dayjs, or regular date object (all of them has toISOString)
((typeof data !== "string" && typeof data.toISOString === "function") ||
// if it's a ISO string already
(typeof data == "string" && isIsoDate(data)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we encode already formatted strings?

Copy link
Contributor Author

@jimmycallin jimmycallin Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we support non-UTC timezones, while the server doesn't. this is to make sure they are in UTC format, since toISOString always return UTC format.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking, do we have a use case to supporting these? It seems like you have to jump through a few hoops to format dates like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing that I'm aware of, I thought it was better to support encoding all ISO8601 compatible strings than just a subset of them.

) {
// wrap it new Date() to convert it to UTC based ISO string in case it is in another timezone
try {
return new Date(data).toISOString();
} catch (err) {
return null;
}
}

return null;
}
Loading