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
Migrate more type tests to TSTyche #1991
Conversation
// No longer works in typescript@>=3.9 | ||
// // $ExpectError - TypeScript does not support Lists as tuples | ||
// Map(List([List(['a', 'b'])])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test was disabled in #1827
I guess dtslint
was running tests on several versions of TypeScript and the test was failing only on some of them. Disabling was the solution. Now it can be enabled.
|
||
// $ExpectType Map<number, number> | ||
const numberMap: Map<number, number> = Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tricky one. It did not make sense to have:
expect<Map<number, number>>().type.toBeAssignable(Map() as Map<number, number>);
I was not sure if it worth keeping this test. It was added in #1827. Perhaps like this:
expect(() => {
const numberMap: Map<number, number> = Map();
}).type.not.toRaiseError();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different from the current test, but it might make sense adding (see comment below):
expect(Map([[1, 'a']])).type.not.toBeAssignableTo<Map<number, number>>();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good enough for me 👍
|
||
const s = Symbol('s'); | ||
test('#getIn', () => { | ||
expect(Map({ a: 4, b: true }).getIn(['a' as const])).type.toBeNumber(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Puzzling one. This and following assertion was mentioned as failing in #1986 as well.
Moving the expression to expect()
changes how types are inferred. Somehow here TypeScript treats the context more mutable as before and infers type of 'a'
as string
(it gets widened). The as const
is required to infer the type correctly.
It is possible to avoid as const
:
const result = Map({ a: 4, b: true }).getIn(['a']);
expect(result).type.toBeNumber();
This works, but it does not mean that user will not hit the problem. So I would prefer testing in more challenging context.
Using the const
type parameter is an elegant solution, but it requires TypeScript 5. I left a TODO comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the const type parameter
of typescript 5.0 does fix this ? (I think this should be the correct fix).
If so, we can keep your test file as you did, and drop TS 5 later and move the const
into the immutable.d.ts file.
expect(Map(List([List(['a', 'b'])]))).type.toEqual< | ||
MapOf<List<List<string>>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, is the resolved type correct? Or something else should be expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this is a realy weird Map construction 😅
Theorically, Map
with a tuple should be Map(collection?: Iterable<[K, V]>): Map<K, V>
So I would think that it should be:
expect(Map(List([List(['a', 'b'])]))).type.toEqual<
Map<List<List<string>>>
>()
It's weird and unexpected to have MapOf
here.
BUT, running that code does give this:
So it may be MapOf({ a: string })
I do not know what to do here, it does seems really weird to have that here 🤔
You may let that commented for future fix.
// No longer works in typescript@>=3.9 | ||
// // $ExpectError - TypeScript does not support Lists as tuples | ||
// OrderedMap(List([List(['a', 'b'])])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// $ExpectError | ||
const invalidNumberSet: Set<number> = Set([1, 'a']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure these are good test (here and similarly in other files). Perhaps? It can be written like this:
expect<Set<number>>().type.not.toBeAssignable(Set([1, 'a']));
, but I don’t like how it reads. Not clear what is being tested.
Here is an idea. I could add .toBeAssignableTo()
and .toBeAssignableFrom()
matchers. Those read well and keeping in mind that .toEqual()
already exists these two sound similar to .toBeGreaterThan()
and .toBeLessThan()
.
And the assertion would look like this:
expect(Set([1, 'a'])).type.not.toBeAssignableTo<Set<number>>();
This looks better. Or?
@jdeniau This PR is ready for review. I managed to migrate all the type tests. Hooray! |
@@ -1 +0,0 @@ | |||
// Minimum TypeScript Version: 4.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tried out: tests are also passing on TS 4.4, but not on TS 4.3.
By the way, DefinitelyTyped is only testing versions of TypeScript that are less than 2 years old. Today this is TypeScript 4.7 and up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can let it like that, but I will break compatibility on minor version for really old version of TS (but I should write it in the documentation)
Great! I'm not at home until Monday, but I will review and merge this next week. Great job 👍 |
Tomorrow I will release TSTyche with the renamed assignability matchers. This PR will be un-drafted after upgrading. |
For the record, I'm currently reviewing this, and this is quite long 😄 Can your create another PR on top of it for the rename matcher change ? |
Sure. Let's do it like this. |
@@ -63,7 +63,7 @@ | |||
"lint:format": "prettier --check \"{__tests__,src,type-definitions,website/src,perf,resources}/**/*{.js,.ts,.tsx,.flow,.css}\"", | |||
"lint:js": "eslint \"{__tests__,src,type-definitions,website/src}/**/*.{js,ts,tsx}\"", | |||
"type-check": "run-s type-check:*", | |||
"type-check:ts": "tsc --project type-definitions/tsconfig.json && tsc --project __tests__/tsconfig.json && dtslint type-definitions/ts-tests --localTs node_modules/typescript/lib", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still useful?
Reading this I think that tstyche should be here more than in test
(like flow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. I thought this could stay as smoke test. Not sure if it does anything significant. Indeed, "type-check:ts": "tstyche"
should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As, à, matter of fact, I did post that some weeks ago, I do not remember why I said that 🤷♂️
@@ -98,7 +98,6 @@ | |||
"benchmark": "2.1.4", | |||
"colors": "1.4.0", | |||
"cpy-cli": "3.1.1", | |||
"dtslint": "^4.2.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
@@ -1 +0,0 @@ | |||
// Minimum TypeScript Version: 4.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can let it like that, but I will break compatibility on minor version for really old version of TS (but I should write it in the documentation)
expect(Map(List([List(['a', 'b'])]))).type.toEqual< | ||
MapOf<List<List<string>>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this is a realy weird Map construction 😅
Theorically, Map
with a tuple should be Map(collection?: Iterable<[K, V]>): Map<K, V>
So I would think that it should be:
expect(Map(List([List(['a', 'b'])]))).type.toEqual<
Map<List<List<string>>>
>()
It's weird and unexpected to have MapOf
here.
BUT, running that code does give this:
So it may be MapOf({ a: string })
I do not know what to do here, it does seems really weird to have that here 🤔
You may let that commented for future fix.
|
||
// $ExpectType Map<number, number> | ||
const numberMap: Map<number, number> = Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good enough for me 👍
|
||
const s = Symbol('s'); | ||
test('#getIn', () => { | ||
expect(Map({ a: 4, b: true }).getIn(['a' as const])).type.toBeNumber(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the const type parameter
of typescript 5.0 does fix this ? (I think this should be the correct fix).
If so, we can keep your test file as you did, and drop TS 5 later and move the const
into the immutable.d.ts file.
{ | ||
// Minimum TypeScript Version: 4.1 | ||
// #getIn | ||
// currently `RetrievePathReducer` does not work with anything else than `MapOf` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// currently `RetrievePathReducer` does not work with anything else than `MapOf` | |
// currently `RetrievePathReducer` does not work with anything else than `MapOf` | |
// TODO : fix this with a better type, it should be resolved to `number` (and not be marked as `fail`) |
Hi @mrazauskas I did merge this with tstyche v2. Thank you for the hard work ! Once again, I find tstyche very promissing, and I will certainly use it over dtslint in future library 👍 Top point for the documentation ! |
Nice to see this merged! Thanks for review. Just to draw attention, in the OP an issue an PR were marked with "closes", but GitHu did not close them. Or you decided to keep these? |
@mrazauskas Yes that's right, it's because I did merged it manually 👍 |
Closes #1985
Closes #1986
This PR migrates all remaining type test to TSTyche.