spectator

spectator copied to clipboard

BREAKING CHANGE: introducing type safety for the template values in createSpyObject and mockProvider. This might cause breaking builds because the compiler will find issues hidden until now.

PR Checklist

Please check if your PR fulfills the following requirements:

• [X] The commit message follows our guidelines: https://github.com/ngneat/spectator/blob/master/CONTRIBUTING.md#commit
• [x] Tests for the changes have been added (for bug fixes / features)
• [x] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[X] Other... Please describe: Stronger type checks for test mock implementation.

What is the current behavior?

Weak type checking when mocking services, only checking the template keys, not the values types.

Issue Number: #607

What is the new behavior?

The TS compiler will show up mismatches between the service template values and the actual service methods.

Does this PR introduce a breaking change?

[X] Yes
[ ] No

Introducing type safety for the template values in createSpyObject and mockProvider. This might cause breaking builds because the compiler will find issues hidden until now.

Other information

Run & review this pull request in StackBlitz Codeflow.

@NetanelBasal Am I supposed to check all PR checklist items (even if they are IMO not relevant) or only the ones that I think are relevant? This PR fixes a bug (sort of) and helps improving code quality. I don't see the need for an additional unit test or documentation, other than the release note explanation of why this is a breaking change. WDYT and how should I move on with this PR?

I'd love to see basic types test. You can use the same library I'm using here

@NetanelBasal Interesting idea and testing library. We spent some hours now to try to find a way to add a meaningful test without testing the type of something that's part of the test itself. On the other hand, the point of the actual change is that the compiler will throw an error if the template type for the provider doesn't match the provider type.

Trying the following things:

import { mockProvider } from '@ngneat/spectator';
import { expectTypeOf } from 'expect-type';

class TestProvider {
  testMethod(a: string): string {
    return '';
  }
}

This would be nice but it doesn't work:

expectTypeOf(mockProvider).parameters.toMatchTypeOf<[TestProvider, { testMethod: (x: string) => string }]>();
expectTypeOf(mockProvider).parameters.not.toMatchTypeOf<[TestProvider, { testMethod: (x: string) => number }]>();

Or this

expectTypeOf(mockProvider).toBeCallableWith(TestProvider, {
  testMethod: (s: string): number => s.length,
});

but .toBeCallableWith never fails, no matter what parameter I give it. To be mentioned, the documentation also doesn't describe what this method is supposed to do.

Would you have a hint what else I could try? Otherwise I suggest taking it as is, the compiler does complain and that's the point, isn't it?

Any ideas/update on this? I think type safety is nice so the community would benefit from this change. @NetanelBasal WDYT?