Migrate more type tests to TSTyche #1991

mrazauskas · 2024-02-29T07:33:27Z

Closes #1985
Closes #1986

This PR migrates all remaining type test to TSTyche.

mrazauskas · 2024-02-29T07:37:19Z

type-definitions/ts-tests/map.ts

-  // No longer works in typescript@>=3.9
-  // // $ExpectError - TypeScript does not support Lists as tuples
-  // Map(List([List(['a', 'b'])]));


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.

mrazauskas · 2024-02-29T07:41:21Z

type-definitions/ts-tests/map.ts


-  // $ExpectType Map<number, number>
-  const numberMap: Map<number, number> = Map();


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();

Different from the current test, but it might make sense adding (see comment below):

expect(Map([[1, 'a']])).type.not.toBeAssignableTo<Map<number, number>>();

That's good enough for me 👍

mrazauskas · 2024-02-29T07:49:58Z

type-definitions/ts-tests/map.ts


-    const s = Symbol('s');
+test('#getIn', () => {
+  expect(Map({ a: 4, b: true }).getIn(['a' as const])).type.toBeNumber();


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.

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.

mrazauskas · 2024-02-29T08:44:54Z

type-definitions/ts-tests/map.ts

+  expect(Map(List([List(['a', 'b'])]))).type.toEqual<
+    MapOf<List<List<string>>>


By the way, is the resolved type correct? Or something else should be expected?

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.

mrazauskas · 2024-02-29T09:15:44Z

type-definitions/ts-tests/ordered-map.ts

  // No longer works in typescript@>=3.9
  // // $ExpectError - TypeScript does not support Lists as tuples
  // OrderedMap(List([List(['a', 'b'])]));


Hm.. I guess type is not correctly resolved here:

$ExpectError does nothing, because there is no error anymore. The lines could be removed or I could add expect.fail(). Only question what is expected type? Is it OrderedMap<string, string>? Hm.. Not that easy to figure this out.

mrazauskas · 2024-02-29T11:41:04Z

type-definitions/ts-tests/set.ts

-  // $ExpectError
-  const invalidNumberSet: Set<number> = Set([1, 'a']);


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?

mrazauskas · 2024-02-29T11:43:42Z

@jdeniau This PR is ready for review. I managed to migrate all the type tests. Hooray!

mrazauskas · 2024-02-29T12:01:02Z

type-definitions/ts-tests/index.d.ts

@@ -1 +0,0 @@
-// Minimum TypeScript Version: 4.5


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.

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)

jdeniau · 2024-02-29T12:05:00Z

@jdeniau This PR is ready for review. I managed to migrate all the type tests. Hooray!

Great! I'm not at home until Monday, but I will review and merge this next week. Great job 👍

mrazauskas · 2024-03-04T05:30:58Z

Tomorrow I will release TSTyche with the renamed assignability matchers. This PR will be un-drafted after upgrading.

jdeniau · 2024-03-04T10:05:42Z

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 ?

mrazauskas · 2024-03-04T10:11:32Z

Can your create another PR on top of it for the rename matcher change ?

Sure. Let's do it like this.

jdeniau · 2024-02-29T17:57:48Z

package.json

@@ -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",


Is this still useful?
Reading this I think that tstyche should be here more than in test (like flow)

Hm.. I thought this could stay as smoke test. Not sure if it does anything significant. Indeed, "type-check:ts": "tstyche" should be enough.

As, à, matter of fact, I did post that some weeks ago, I do not remember why I said that 🤷‍♂️

jdeniau · 2024-02-29T17:57:59Z

package.json

@@ -98,7 +98,6 @@
    "benchmark": "2.1.4",
    "colors": "1.4.0",
    "cpy-cli": "3.1.1",
-    "dtslint": "^4.2.1",


jdeniau · 2024-02-29T18:00:37Z

type-definitions/ts-tests/index.d.ts

@@ -1 +0,0 @@
-// Minimum TypeScript Version: 4.5


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)

type-definitions/ts-tests/list.ts

jdeniau · 2024-03-04T09:34:18Z

type-definitions/ts-tests/map.ts

+  expect(Map(List([List(['a', 'b'])]))).type.toEqual<
+    MapOf<List<List<string>>>


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.

jdeniau · 2024-03-04T09:36:14Z

type-definitions/ts-tests/map.ts


-  // $ExpectType Map<number, number>
-  const numberMap: Map<number, number> = Map();


That's good enough for me 👍

jdeniau · 2024-03-04T09:49:01Z

type-definitions/ts-tests/map.ts


-    const s = Symbol('s');
+test('#getIn', () => {
+  expect(Map({ a: 4, b: true }).getIn(['a' as const])).type.toBeNumber();


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.

jdeniau · 2024-03-04T09:54:44Z

type-definitions/ts-tests/map.ts

-{
-  // Minimum TypeScript Version: 4.1
-  // #getIn
+  // currently `RetrievePathReducer` does not work with anything else than `MapOf`


Suggested change

// 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`)

type-definitions/ts-tests/ordered-set.ts

jdeniau · 2024-04-15T20:26:35Z

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 !

mrazauskas · 2024-04-16T04:56:03Z

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?

jdeniau · 2024-04-16T07:25:58Z

@mrazauskas Yes that's right, it's because I did merged it manually 👍

chore: migrate to TSTyche

c36e2eb

mrazauskas commented Feb 29, 2024

View reviewed changes

refactor OrderedMap tests

1cc862c

mrazauskas commented Feb 29, 2024

View reviewed changes

mrazauskas added 9 commits February 29, 2024 11:48

refactor OrderedSet tests

7c4bf20

refactor partition tests

cde2fed

refactor Range tests

be17032

refactor Record tests

9117c63

refactor Repeat tests

c79947e

refactor Seq tests

4772a82

refactor Set tests

c87e525

refactor Stack tests

c3f4e5d

clean up

16bdcf7

mrazauskas marked this pull request as ready for review February 29, 2024 11:31

mrazauskas commented Feb 29, 2024

View reviewed changes

mrazauskas marked this pull request as draft March 4, 2024 05:29

mrazauskas marked this pull request as ready for review March 4, 2024 10:11

jdeniau reviewed Apr 15, 2024

View reviewed changes

jdeniau merged commit 16bdcf7 into immutable-js:5.x Apr 15, 2024
5 checks passed

mrazauskas deleted the migrate-to-tstyche branch April 16, 2024 04:11

jdeniau mentioned this pull request Apr 16, 2024

Migrate from dtslint to tsd #1986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate more type tests to TSTyche #1991

Migrate more type tests to TSTyche #1991

mrazauskas commented Feb 29, 2024 •

edited

mrazauskas Feb 29, 2024

mrazauskas Feb 29, 2024 •

edited

mrazauskas Feb 29, 2024

jdeniau Mar 4, 2024

mrazauskas Feb 29, 2024 •

edited

jdeniau Mar 4, 2024

mrazauskas Feb 29, 2024

jdeniau Mar 4, 2024

mrazauskas Feb 29, 2024 •

edited

mrazauskas Feb 29, 2024 •

edited

mrazauskas commented Feb 29, 2024

mrazauskas Feb 29, 2024 •

edited

jdeniau Feb 29, 2024

jdeniau commented Feb 29, 2024

mrazauskas commented Mar 4, 2024

jdeniau commented Mar 4, 2024

mrazauskas commented Mar 4, 2024

jdeniau Feb 29, 2024

mrazauskas Apr 16, 2024

jdeniau Apr 16, 2024

jdeniau Feb 29, 2024

jdeniau Feb 29, 2024

jdeniau Mar 4, 2024

jdeniau Mar 4, 2024

jdeniau Mar 4, 2024

jdeniau Mar 4, 2024

jdeniau commented Apr 15, 2024

mrazauskas commented Apr 16, 2024

jdeniau commented Apr 16, 2024


		// $ExpectType Map<number, number>
		const numberMap: Map<number, number> = Map();

		expect(Map(List([List(['a', 'b'])]))).type.toEqual<
		MapOf<List<List<string>>>

		// $ExpectError
		const invalidNumberSet: Set<number> = Set([1, 'a']);

	// 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`)

Migrate more type tests to TSTyche #1991

Migrate more type tests to TSTyche #1991

Conversation

mrazauskas commented Feb 29, 2024 • edited

Choose a reason for hiding this comment

mrazauskas Feb 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrazauskas Feb 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrazauskas Feb 29, 2024 • edited

Choose a reason for hiding this comment

mrazauskas Feb 29, 2024 • edited

Choose a reason for hiding this comment

mrazauskas commented Feb 29, 2024

mrazauskas Feb 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdeniau commented Feb 29, 2024

mrazauskas commented Mar 4, 2024

jdeniau commented Mar 4, 2024

mrazauskas commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdeniau commented Apr 15, 2024

mrazauskas commented Apr 16, 2024

jdeniau commented Apr 16, 2024

mrazauskas commented Feb 29, 2024 •

edited

mrazauskas Feb 29, 2024 •

edited

mrazauskas Feb 29, 2024 •

edited

mrazauskas Feb 29, 2024 •

edited

mrazauskas Feb 29, 2024 •

edited

mrazauskas Feb 29, 2024 •

edited