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鈥檒l 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

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 this to the July 2019 milestone milestone Jul 1, 2019

@bajtos bajtos requested a review from strongloop/loopback-maintainers Jul 1, 2019

@bajtos bajtos requested a review from raymondfeng as a code owner Jul 1, 2019

@bajtos bajtos self-assigned this Jul 1, 2019

@hacksparrow
Copy link
Member

left a comment

Why use Math.random()?

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

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

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow Jul 1, 2019

Member

Wouldn't this always yield {}?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 1, 2019

Member

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.

This comment has been minimized.

Copy link
@hacksparrow

hacksparrow Jul 1, 2019

Member

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

});

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 1, 2019

Member

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 1, 2019

Member

For example:

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

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 Jul 2, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

@raymondfeng @hacksparrow updated, LGTY now?

@bajtos bajtos requested review from raymondfeng and hacksparrow Jul 2, 2019

@hacksparrow

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

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

feat(testlab): improve typings for `toJSON()` helper
Let the compiler understand toJSON's behavior for commonly used
union types like `object | null` or `unknown[] | null`.

Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>

@bajtos bajtos force-pushed the feat/testlab-tojson-types branch from 04c4307 to 7bd2264 Jul 4, 2019

@bajtos bajtos merged commit a64e860 into master Jul 4, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 91.496%
Details

@delete-merged-branch delete-merged-branch bot deleted the feat/testlab-tojson-types branch Jul 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.