Conversation
✅ Deploy Preview for ngrx-io ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
| expect< | ||
| Extract<keyof typeof arrayState, string | number> | ||
| >().type.toBe<never>(); |
There was a problem hiding this comment.
Here and in other similar cases I adapted the approached from #5127. Although the assertion looks strange. The meaning of it is unclear. But it might be I do not understand the context well.
Originally it was tested wherever keyof typeof arrayState is inferred as unique symbol | unique symbol. Is also puzzling. Each unique symbol is only identical to itself. It cannot be identical no other type including abstract unique symbol type. Feels like the union should have names of two symbols. But it does not.
For instance, in the below only STATE_SOURCE will work. It cannot be replaced with unique symbol. But in the test above, the inferred type of keyof typeof arrayState is unique symbol | typeof STATE_SOURCE.
And what bothers me in the .toBe<never>() test is that this tests what the type does not do instead of what it does.
| const snippet = ` | ||
| const state = signalState(() => {}); | ||
| const stateValue = state(); | ||
| declare const stateKeys: keyof typeof state; | ||
| `; | ||
|
|
||
| const result = expectSnippet(snippet); | ||
| result.toInfer('stateValue', '() => void'); | ||
| result.toInfer('stateKeys', 'unique symbol | unique symbol'); | ||
| const state = signalState(() => {}); | ||
| const stateValue = state(); | ||
|
|
||
| expect(stateValue).type.toBe<() => void>(); | ||
| expect<Extract<keyof typeof state, string | number>>().type.toBe<never>(); |
There was a problem hiding this comment.
Another thought about the unique symbols. What about rewriting this and similar tests like this:
const state = signalState(() => {});
expect(state).type.toBeAssignableTo<StateSource<() => void>>()
expect(state).type.toBeAssignableTo<Signal<() => void>>()
const stateValue = state();
expect(stateValue).type.toBe<() => void>();The idea is to replace result.toInfer('stateKeys', 'unique symbol | unique symbol') test, with assignability checks. These proof that state can be used where StateSource or Signal is accepted. Feels like this could be closer to a user story. And also an indirect check that state has the [STATE_SOURCE] and [SIGNAL] keys.
|
Dear @mrazauskas, I'm closing this PR. Tooling preference: We wanted to see whether we could handle type tests by relying solely on Vitest and @ts-expect-error. TSTyche comes with additional benefits, but in that case, it doesn't outweigh the cost of adding another dependency. Contribution policy: We're pretty strict about our policy which requires an issue and agreement before a PR is opened. This specific task (issue #4935) was already assigned to Santosh. Again, thanks for your contribution, but this time we can't accept it. |
|
I understand. Thanks for taking the time to explain. |
2 participants
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
ts-snippetis is currently used to test types, but there are some preseasons to replace it. See #4935What is the new behavior?
This PR migrates
@ngrx/signalstype tests to usetstyche.Related #5127
Does this PR introduce a breaking change?
Other information
Based on #4935 (comment), it felt like there is interest in trying out TSTyche. I am its author.
TSTyche is a standalone type test runner. It wraps around TypeScript’s programmatic APIs. This means the types are compared programmatically and additional static analysis is possible. Therefore, TSTyche is able to cover more edge cases, check error messages suppressed by
@ts-expect-error, or test against specific TypeScript versions.It also looks and feels like a test runner. Check out this branch and try:
pnpm tstyche, runs all test filespnpm tstyche uprotected, runs a single filepnpm tstyche --target 6.0, tests against TypeScript 6.0 (and reports problems with TSConfig)pnpm tstyche --target '>=5.8', tests against a range of TS versionsIt must be noted that TypeScript 7 is not yet supported, because their programatic API is not yet ready. There are no blockers on TSTyche side.
Few notes on this migration:
.not.toBeCallableWith()matcher or@ts-expect-errordirective. The matcher checks if there is an error when the expression is called with given arguments; if the directive is used, TSTyche checks if the provided and suppressed error message matches. I used the matcher for simple test cases and the directive for complex ones.tsconfig.jsonfiles were added to create isolated test projects. Otherwise, fixtures and other unrelated files get type checked. That felt unnecessary.nxand I don’t know how to set it up well. Any guidance would be very helpful.