Skip to content

Conversation

@bajtos
Copy link
Member

@bajtos bajtos commented Sep 20, 2018

Implement a helper function to convert an object (e.g. a Model instance) to a data object representing JSON version of the input object.

Specifically, properties set to a value that cannot be represented in
JSON (e.g. undefined) are removed from the result.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site testlab's README was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

// License text available at https://opensource.org/licenses/MIT

/**
* JSON transport does not preserve properties that are undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

We should say JSON format or JSON encoding instead of transport.

* Use this function to convert a model instance into a data object
* as returned by REST API
*/
export function deserializedFromJson<T extends object>(value: T): T {
Copy link
Contributor

Choose a reason for hiding this comment

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

The method name is a bit misleading. It tricks me to think about the argument is a JSON string.

What about asJSON() or toJSON()?

Do we want to restrain the argument to be an object? What about any?

We need to deal with the case where value is undefined.

JSON.parse(JSON.stringify(undefined)); // SyntaxError: Unexpected token u in JSON at position 0

The return type is incorrect, for example:

// Customer is a class with `getFullName()` method.
const customer: Customer = await this.repo.findById('c001');
const json = deserializedFromJson(customer); // TypeScript infers the return type as `Customer`
json.getFullName(); // The function does not exist but TypeScript cannot catch it.

I propose that we change the method to be:

export function toJSON<T extends object>(value: T): DeepPartial<T> { // ... }

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to using either asJSON or toJSON ... with a slight preference for toJSON.

// Customer is a class with `getFullName()` method.
const customer: Customer = await this.repo.findById('c001');
const json = deserializedFromJson(customer); // TypeScript infers the return type as `Customer`
json.getFullName(); // The function does not exist but TypeScript cannot catch it.

I don't think json.getFullName(); is a valid use of this as the intention of this method is to act as a helper to allow comparison of an object to the value you would get from a rest api ... which is the use case for testing (and the inspiration for this helper method). One should just do customer.getFullName() instead of converting to JSON and then trying to treat it as a class.

Considering the above maybe a better name we can use is modelToJSON?

Copy link
Contributor

Choose a reason for hiding this comment

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

@virkt25 Please note by the current signature, deserializedFromJson(customer); returns a Customer, which has getFullName(). As a result, TypeScript thinks json.getFullName() is valid use but the getFullName does not exist on the object.

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

Implementation LGTM. Just need to agree on a name as per the comments.

@bajtos bajtos force-pushed the feat/deserializedFromJson branch from 5bcce0c to 3d92cfa Compare September 21, 2018 07:46
@bajtos
Copy link
Member Author

bajtos commented Sep 21, 2018

export function toJSON<T extends object>(value: T): DeepPartial<T> { // ... }

I think this isn't going to really fix the problem, because DeepPartial will only make all methods optional - it is not going to hide them from the result.

I solved the problem by returning a generic object type. This type does not have any properties nor methods beyond built-in Object.prototype. This is good enough for the use case of comparing data via deep-equal.

I have also renamed the helper function to toJSON and added support for other value types, from undefined and null to scalar types like numbers & strings.

PTAL again, LGTY now?

@raymondfeng raymondfeng changed the title feat(testlab): add a new helper deserializedFromJson feat(testlab): add a new helper toJSON Sep 21, 2018
@raymondfeng
Copy link
Contributor

@bajtos Please address my latest comments.

@raymondfeng
Copy link
Contributor

So far, @loopback/testlab does not depend on @loopback/repository. Are we ok with adding this dependency?

No need to use @model. Just plain JS classes will do.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Implement a helper function to convert an object (e.g. a Model instance)
to a data object representing JSON version of the input object.
Specifically, properties set to a value that cannot be represented in
JSON (e.g. `undefined`) are removed from the result.
@bajtos bajtos force-pushed the feat/deserializedFromJson branch from 3d92cfa to e7a9ef4 Compare September 24, 2018 06:50
@bajtos
Copy link
Member Author

bajtos commented Sep 24, 2018

@raymondfeng added more tests as you requested and simplified the actual implementation by wrapping the input value in an object property. LGTY now?

Copy link
Contributor

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

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

Great work!

@raymondfeng raymondfeng merged commit 8d105e4 into master Sep 24, 2018
@raymondfeng raymondfeng deleted the feat/deserializedFromJson branch September 24, 2018 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants