fix(typescript): Fix incorrect type generated for glob route parameters | Add type tests by dac09 · Pull Request #6364 · redwoodjs/graphql

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

* '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

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

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

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

@@ -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 <[email protected]>

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

Comment thread packages/router/src/__typetests__/routeParamsTypes.test.ts Outdated

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

Comment thread packages/router/src/routeParamsTypes.ts Outdated

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 <[email protected]>

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

Conversation

dac09 commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dac09 commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dac09 commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mrazauskas Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dac09 Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

dac09 Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrazauskas Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dac09 Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

mrazauskas Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

mrazauskas Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

dac09 Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dac09 Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrazauskas Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrazauskas Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tobbe commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimenB commented Sep 16, 2022

Uh oh!

dac09 commented Sep 16, 2022

Uh oh!

SimenB commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dac09 commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrazauskas commented Sep 16, 2022

Uh oh!

dac09 commented Sep 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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