fix(typescript): Fix incorrect type generated for glob route parameters | Add type tests #6364

dac09 · 2022-09-08T17:25:03Z

This PR uses https://github.com/jest-community/jest-runner-tsd to add type tests

This is helpful because we currently have no way of checking complicated mapper types, apart from manually generating a test project, writing some code, and running tsc (or waiting for red)

Todo:

Fix failing test for glob route strings - this is an actual bug
~~- [ ] Add tests for MergePrismaWithSdlTypes~~ Will do in another PR
@Tobbe thoughts?

Closes #6346
Closes #6319

packages/router/src/__typetests__/routeParamsTypes.test.ts

* 'main' of github.com:redwoodjs/redwood: fix(deps): update dependency @graphql-yoga/common to v2.12.12 (redwoodjs#6349) fix(test-project): revert @redwoodjs/core to rc Update yarn.lock v2.2.4 bugfix replace slash in tailwind config on windows (redwoodjs#6203) bugfix replace slash in tailwind config on windows (redwoodjs#6203) chore(deps): update dependency @testing-library/dom to v8.17.1 (redwoodjs#6351) Update yarn.lock Use try/catch to access unauthenticated (redwoodjs#6358) issue#5852 added windows fix for nodeFileTrace (redwoodjs#6325) Handle special props `ref` and `key` in path and search params (redwoodjs#5537) Use try/catch to access unauthenticated (redwoodjs#6358) feat(codemod): Add codemod to make relation resolvers partial (redwoodjs#6342)

dac09 · 2022-09-09T12:22:59Z

Questions:

do we allow routes to start with a parameter? e.g. /{id}/bazinga (I assume no, right? That seems odd)
do we allow routes to start with with a glob? e.g. /{...globParam}/ffff (how would we know where to start/stop?)

Switch to expectAssignable

dac09 · 2022-09-09T15:52:32Z

@Tobbe - unbelievably I made the glob thing work too - but I broke down the types into parts that will be atleast a little bit more grokable. Please do suggest better names - I notice I'm mixing up the "verb form" e.g. RemoveGlobDots and the "noun form" e.g. MultiParamsWithoutType. Aside adding Parse to the front of the noun forms, is there a convention we should follow?

Please let me know what you think when you have a chance to look - maybe I'll add the tests for the PrismaMerge thing in another pr!

because *.d.ts don't get copied to dist

packages/router/src/routeParamsTypes.ts

mrazauskas · 2022-09-12T13:40:37Z

packages/router/src/__typetests__/tsconfig.json

@@ -0,0 +1,3 @@
+{
+  "extends": "../../tsconfig.json"


Try adding the following lines and importing from .ts file. Does it help?

"compilerOptions": { "composite": false, }

Thanks for looking into it!

Yep I had to set composite, but also set incremental - because the base tsconfig contains tsBuildInfo - which needs one of these flags. I'm not 100% sure the impact this has though, but since its only for the tsd test environment should be ok, I think

{ "extends": "../../tsconfig.json", "compilerOptions": { "composite": false, "incremental": true }, }

Actually had to add all of these 😢 - because of conflicting flags set in the original config. I suppose these settings only affect the build - which we're not in the case of type tests

{ "extends": "../../tsconfig.json", "compilerOptions": { "composite": false, "incremental": true, "declaration": false, "declarationMap": false, "emitDeclarationOnly": false }, }

Is the fact that the tsconfig in the tests drifting away from the project config an issue?

You're right about composite definitely being the problem... but if I check the config with tsc here:

❯ yarn tsc -p src/__typetests__ --showConfig { "compilerOptions": { "target": "esnext", "moduleResolution": "node", "module": "esnext", "declarationMap": true, "emitDeclarationOnly": true, "sourceMap": true, "composite": true, "strict": true, "esModuleInterop": true, "noUnusedLocals": true, "noUnusedParameters": true, "noImplicitReturns": true, "noFallthroughCasesInSwitch": true, "jsx": "preserve", "skipLibCheck": true, "resolveJsonModule": true, "tsBuildInfoFile": "../../dist/tsconfig.tsbuildinfo", "baseUrl": "../..", "rootDir": "..", "outDir": "../../dist", "paths": { "src/*": [ "./src/*" ] } }, "files": [ "../ActivePageContext.tsx", "../PageLoadingContext.tsx", "../Set.tsx", "../a11yUtils.ts", "../active-route-loader.tsx", "../history.tsx", "../index.ts", "../links.tsx", "../location.tsx", "../params.tsx", "../route-announcement.tsx", "../route-focus.tsx", "../routeParamsTypes.ts", 👈 definitely here "../router-context.tsx", "../router.tsx", "../splash-page.tsx", "../useIsMounted.ts", "../util.ts", "../../ambient.d.ts" ], "include": [ "../../src", "../.././ambient.d.ts" ], "exclude": [ "../../../../dist", "../../../../node_modules", "../../../../**/__tests__", "../../../../**/__mocks__", "../../../../**/*.test.*" ] }

Yes, the file is included. I see the same config from inside tsd-lite. It is using TS compiler’s APIs to find and read config.

The error we saw comes from TS compiler and it looks misleading. Here is an experiment:

Add empty tsconfig.json to __typetests__ folder.

Run yarn tsc -p src/__typetests__. All works.

Add "compilerOptions": {"composite": true} to the same tsconfig.json and run the same command.

Oh.. that’s the error we saw.. And it came from tsc this time.

Now also add "references": [{ "path": "../../" }] to the same tsconfig.json and run the command.

All works again. Isn’t it that with "composite": true each directory with tsconfig.json turns into a project? So in this case the compiler complains, because it sees src and __typetests__ as separate projects.

By the way, tsd-lite wraps TS compilers’s APIs to typecheck and compare types. If tsc complains, tsd-lite will complain in the same way. EDIT: Only that tsd-lite compiles in memory and does not emit anything, so these extra emit options have no impact.

Thanks for the explanation Tom, makes sense.

The TS docs lack a little bit on composite and incremental - but it certainly seems to behave the way you're describing. I'll close the issue on the jest runner side :)

Wait. I just realised that tsc needs -b flag to work with composites, not -p. So error comes from something else. Hm.. Just tried to use yarn tsc -b src/__typetests__ with previous experiment. Same result.

Back to your question, tsd-lite looks for a tsconfig.json nearest to a test file. If not found, it will use defaults of the compiler. If you delete __typetests__/tsconfig.json, tsd-lite will use config from packages/router. It gives the same error, adding {"composite": false} to packages/router/tsconfig.json makes it work.

Very interesting case. I have to dig deeper into this to understand. In any case, since tsc throws the same error this must be a config issue.

Can’t stop ;D I just looked at Jest repo where tsd-lite is used (with 1200+ assertions passing) and in __typetests__/tsconfig.json we have "noUnusedLocals": false, "noUnusedParameters": false, etc. In a larger test suite these are useful. With time it becomes handy to have dedicated tsconfig.json for type tests, although it might look strange at first.

I mean to ask a much simpler question, why doesn't tsd-lite work with composite? Just for knowledge - I don't quite grasp what the composite flag is for (outside of a build-time optimisation). I wouldn't be surprised if it's just a misconfiguration on our side.

Thank you very much for looking into it - glad it's piqued your interest!

Oh my - I think I spotted the problem! Look at the last exclude glob, in the output for the tsc config it ignores the test files 😅

Right. I was looking at that glob. Might be the cause, but note that routeParamsTypes.ts is reported as not included, which is not a test file. Also you need those globs for build. So they have to stay.

I think tsd-lite should work with composites, but with __typetests__/tsconfig.json it needs references. Just like tsc in my experiment, which I described above. At the same time, composite is useful for building libraries, there is not much use of it in testing. Perhaps there is a use case, but I think "composite": false is better solution in your case.

Import type file directly

packages/router/src/__typetests__/routeParamsTypes.test.ts

mrazauskas · 2022-09-12T17:14:37Z

packages/router/src/__typetests__/tsconfig.json

@@ -0,0 +1,3 @@
+{
+  "extends": "../../tsconfig.json"


Yes, the file is included. I see the same config from inside tsd-lite. It is using TS compiler’s APIs to find and read config.

The error we saw comes from TS compiler and it looks misleading. Here is an experiment:

Add empty tsconfig.json to __typetests__ folder.

Run yarn tsc -p src/__typetests__. All works.

Add "compilerOptions": {"composite": true} to the same tsconfig.json and run the same command.

Oh.. that’s the error we saw.. And it came from tsc this time.

Now also add "references": [{ "path": "../../" }] to the same tsconfig.json and run the command.

All works again. Isn’t it that with "composite": true each directory with tsconfig.json turns into a project? So in this case the compiler complains, because it sees src and __typetests__ as separate projects.

By the way, tsd-lite wraps TS compilers’s APIs to typecheck and compare types. If tsc complains, tsd-lite will complain in the same way. EDIT: Only that tsd-lite compiles in memory and does not emit anything, so these extra emit options have no impact.

Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>

Tobbe · 2022-09-15T12:09:21Z

Questions:

do we allow routes to start with a parameter? e.g. /{id}/bazinga (I assume no, right? That seems odd)

do we allow routes to start with with a glob? e.g. /{...globParam}/ffff (how would we know where to start/stop?)

Both of those are allowed, and supported. I added a couple of (regular) tests to make it more explicit

SimenB · 2022-09-16T14:01:18Z

👏

dac09 · 2022-09-16T14:02:04Z

Hello @SimenB good to see you around here :)

SimenB · 2022-09-16T14:03:12Z

Love seeing jest-runner-tsd get more users 😀 Don't hesitate to reach out of it's behaving weird or is missing features - AFAIK this is the first large project outside of Jest that uses it 🙂

dac09 · 2022-09-16T14:09:30Z

Love seeing jest-runner-tsd get more users 😀 Don't hesitate to reach out of it's behaving weird or is missing features - AFAIK this is the first large project outside of Jest that uses it 🙂

It's a great project, although a little hard to search for (because testing typescript just returns a bunch of articles how to configure jest for ts!)

One thing that feels a little ~~painful~~ unintuitive is that I need to pass a constant to expectType. Since most of our use cases will be checking mapper types, it'd be great to just check types without needing a const/var assignment.

But I also understand why it's this way :)

mrazauskas · 2022-09-16T14:22:13Z

feels a little painful is that I need to pass a constant to expectType

That’s a limitation of tsd unfortunately, but there are good news in this field. I work on alternative testing tool with more flexible API (also describe, it, test are part of the API, no need to borrow them from Jest):

test("is ExpectedType?", () => {
  expect<SomeType>().type.toBe<ExpectedType>();
});

test("is typeof expectedExpression?", () => {
  expect(receivedExpression).type.toBe(expectedExpression);
});

And everything in between ;D Actually the above example is already fully working. I just have to cleaning up TODOs and to write more docs. Moving forward slowly.

If that makes sense, I will ping you after releasing this beauty (; Or just follow: jest-community/jest-runner-tsd#32

dac09 · 2022-09-16T14:23:52Z

Following!

This is exactly what I was hoping the syntax would be. I’m a big fan of tests reading like documentation, and this is definitely a step in the right direction. Thank you both for your hard work on this (and jest) 👍👍👍👍

dac09 added 3 commits September 6, 2022 18:22

Move route parser to inside rwjsRouter

3330c73

Simple tsdlite setup with jest

09e462e

Add tsd tests for routeParamsTypes

6c86058

redwoodjs-bot bot assigned dac09 Sep 8, 2022

Don't use caret

73d5906

dac09 commented Sep 8, 2022

View reviewed changes

packages/router/src/__typetests__/routeParamsTypes.test.ts Outdated Show resolved Hide resolved

dac09 added the release:fix This PR is a fix label Sep 8, 2022

dac09 added 2 commits September 8, 2022 18:33

Dedupe yarn

b98bba6

dac09 added 4 commits September 9, 2022 13:55

Break up RouteParser, add aditional tests

7ca4f78

Fix glob type |

fbd69a0

Switch to expectAssignable

Comment

e671ab2

Merge branch 'main' into try/tsd-tests

d870521

dac09 marked this pull request as ready for review September 9, 2022 15:52

Rename one of the types

684f7c0

dac09 changed the title ~~Add tsd tests for types~~ fix(typescript): Fix incorrect type generated for glob route parameters | Add type tests Sep 11, 2022

dac09 added 2 commits September 12, 2022 11:52

Merge main

806d120

Use a ts file for types instead,

50a7671

because *.d.ts don't get copied to dist

dac09 commented Sep 12, 2022

View reviewed changes

packages/router/src/routeParamsTypes.ts Outdated Show resolved Hide resolved

Update packages/router/src/routeParamsTypes.ts

1845e62

mrazauskas reviewed Sep 12, 2022

View reviewed changes

Add extra tsconfig to typetests |

6fdeae5

Import type file directly

mrazauskas reviewed Sep 12, 2022

View reviewed changes

Update packages/router/src/__typetests__/routeParamsTypes.test.ts

88edc39

Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>

mrazauskas mentioned this pull request Sep 12, 2022

File is not listed within the file list of project '.../__typetests__/tsconfig.json' jest-community/jest-runner-tsd#111

Closed

Merge branch 'main' into try/tsd-tests

d860055

Add a couple of more router matchPath tests

061bcc8

dac09 added 6 commits September 15, 2022 15:50

Merge branch 'main' into try/tsd-tests

8706a05

Add more tests

e68fb7f

Fix the strange edge cases

64758f6

Use expectType in a different way for more accurate tests

c1d424f

Add FAQ to tests

9e3f586

tweak messages

37e1c79

Tobbe approved these changes Sep 16, 2022

View reviewed changes

Merge branch 'main' into try/tsd-tests

7359f8c

Tobbe enabled auto-merge (squash) September 16, 2022 12:18

Merge branch 'main' into try/tsd-tests

b80f2d2

Tobbe merged commit 22c3d1d into redwoodjs:main Sep 16, 2022

redwoodjs-bot bot added this to the next-release milestone Sep 16, 2022

dac09 deleted the try/tsd-tests branch September 16, 2022 14:02

jtoar modified the milestones: next-release, v3.1.0 Sep 28, 2022

mrazauskas mentioned this pull request Nov 10, 2023

chore: migrate type tests to TSTyche #9394

Merged

Tobbe mentioned this pull request Jan 31, 2024

fix(internal): Remove unused import #9946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(typescript): Fix incorrect type generated for glob route parameters | Add type tests #6364

fix(typescript): Fix incorrect type generated for glob route parameters | Add type tests #6364

dac09 commented Sep 8, 2022 •

edited

Loading

dac09 commented Sep 9, 2022 •

edited

Loading

dac09 commented Sep 9, 2022 •

edited

Loading

mrazauskas Sep 12, 2022 •

edited

Loading

dac09 Sep 12, 2022

dac09 Sep 12, 2022 •

edited

Loading

mrazauskas Sep 12, 2022 •

edited

Loading

dac09 Sep 12, 2022

mrazauskas Sep 12, 2022

mrazauskas Sep 12, 2022

dac09 Sep 12, 2022 •

edited

Loading

dac09 Sep 12, 2022 •

edited

Loading

mrazauskas Sep 13, 2022

mrazauskas Sep 12, 2022 •

edited

Loading

Tobbe commented Sep 15, 2022 •

edited

Loading

SimenB commented Sep 16, 2022

dac09 commented Sep 16, 2022

SimenB commented Sep 16, 2022 •

edited

Loading

dac09 commented Sep 16, 2022 •

edited

Loading

mrazauskas commented Sep 16, 2022

dac09 commented Sep 16, 2022

fix(typescript): Fix incorrect type generated for glob route parameters | Add type tests #6364

fix(typescript): Fix incorrect type generated for glob route parameters | Add type tests #6364

Conversation

dac09 commented Sep 8, 2022 • edited Loading

dac09 commented Sep 9, 2022 • edited Loading

dac09 commented Sep 9, 2022 • edited Loading

mrazauskas Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

dac09 Sep 12, 2022

Choose a reason for hiding this comment

dac09 Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

mrazauskas Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

dac09 Sep 12, 2022

Choose a reason for hiding this comment

mrazauskas Sep 12, 2022

Choose a reason for hiding this comment

mrazauskas Sep 12, 2022

Choose a reason for hiding this comment

dac09 Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

dac09 Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

mrazauskas Sep 13, 2022

Choose a reason for hiding this comment

mrazauskas Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Tobbe commented Sep 15, 2022 • edited Loading

SimenB commented Sep 16, 2022

dac09 commented Sep 16, 2022

SimenB commented Sep 16, 2022 • edited Loading

dac09 commented Sep 16, 2022 • edited Loading

mrazauskas commented Sep 16, 2022

dac09 commented Sep 16, 2022

dac09 commented Sep 8, 2022 •

edited

Loading

dac09 commented Sep 9, 2022 •

edited

Loading

dac09 commented Sep 9, 2022 •

edited

Loading

mrazauskas Sep 12, 2022 •

edited

Loading

dac09 Sep 12, 2022 •

edited

Loading

mrazauskas Sep 12, 2022 •

edited

Loading

dac09 Sep 12, 2022 •

edited

Loading

dac09 Sep 12, 2022 •

edited

Loading

mrazauskas Sep 12, 2022 •

edited

Loading

Tobbe commented Sep 15, 2022 •

edited

Loading

SimenB commented Sep 16, 2022 •

edited

Loading

dac09 commented Sep 16, 2022 •

edited

Loading