Skip to content
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.

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

feat(testlab): improve typings for toJSON() helper #3283

Merged
merged 1 commit into from
Jul 4, 2019

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Jul 1, 2019

While working on #2634, I discovered that findOne returns T | null and this type cannot be passed to toJSON helper, because there is no overload accepting such type.

This pull requests improves the type definitions to let the compiler understand toJSON's behavior for commonly used union types like object | null or unknown[] | null.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • 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 was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@bajtos bajtos added feature Testlab @loopback/testlab labels Jul 1, 2019
@bajtos bajtos added this to the July 2019 milestone milestone Jul 1, 2019
@bajtos bajtos requested a review from a team July 1, 2019 13:49
@bajtos bajtos requested a review from raymondfeng as a code owner July 1, 2019 13:49
@bajtos bajtos self-assigned this Jul 1, 2019
Copy link
Contributor

@hacksparrow hacksparrow left a comment

Choose a reason for hiding this comment

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

Why use Math.random()?

@@ -62,6 +62,50 @@ describe('toJSON', () => {
expectUndefined(value);
});

it('handles `object | null`', () => {
const input: object | null = Math.random() ? {} : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this always yield {}?

Copy link
Contributor

Choose a reason for hiding this comment

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

The Math.random() function returns a floating-point, pseudo-random number in the range 0–1 (inclusive of 0, but not 1) with approximately uniform distribution over that range

I think Math.random() ? returns true in most cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, so the behavior of input = null will rarely get tested.

});

it('handles `object | undefined`', () => {
const input: object | undefined = Math.random() ? {} : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract Math.random() ? to a helper function, such as: randomChoice(...values: any[]): any

Copy link
Contributor

Choose a reason for hiding this comment

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

For example:

function randomChoice(...values: any[]): any {
  if (!values.length) return undefined;
  const index = Math.floor(Math.random() * values.length);
  return values[index];
}

@bajtos
Copy link
Member Author

bajtos commented Jul 2, 2019

Thank you @raymondfeng and @hacksparrow for your comments. I think I really need to improve the tests to make it more clear what is going on and what is being tested. In the tests, I don't really care about the actual value being converted via toJSON, because the implementation is always the same. What I need: compile-time checks.

@bajtos
Copy link
Member Author

bajtos commented Jul 2, 2019

The main problem is that from what I can tell by observing type hints offered by VSCode, the following statement is optimized by TypeScript and a simpler type is used instead.

const input: object | null = {};
const output = toJSON(input);
// VSCode shows the following hint:
// (alias) toJSON(value: object): object (+14 overloads)
// import toJSON

Compare to my current version:

const input: object | null = Math.random() ? {} : null;
const output = toJSON(input);
// VSCode shows the following hint:
// (alias) toJSON(value: object | null): object | null (+14 overloads)
// import toJSON

@bajtos bajtos force-pushed the feat/testlab-tojson-types branch from 6b25603 to 04c4307 Compare July 2, 2019 10:24
@bajtos
Copy link
Member Author

bajtos commented Jul 2, 2019

@raymondfeng @hacksparrow updated, LGTY now?

@hacksparrow
Copy link
Contributor

LGTM if it's just for compile-time TS checks.

Let the compiler understand toJSON's behavior for commonly used
union types like `object | null` or `unknown[] | null`.

Signed-off-by: Miroslav Bajtoš <[email protected]>
@bajtos bajtos force-pushed the feat/testlab-tojson-types branch from 04c4307 to 7bd2264 Compare July 4, 2019 06:50
@bajtos bajtos merged commit a64e860 into master Jul 4, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/testlab-tojson-types branch July 4, 2019 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Testlab @loopback/testlab
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants