Skip to content
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

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

Merged
merged 26 commits into from
Sep 16, 2022

Conversation

dac09
Copy link
Collaborator

@dac09 dac09 commented Sep 8, 2022

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

@dac09 dac09 added the release:fix This PR is a fix label Sep 8, 2022
* '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
Copy link
Collaborator Author

dac09 commented Sep 9, 2022

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?)

@dac09
Copy link
Collaborator Author

dac09 commented Sep 9, 2022

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

@dac09 dac09 marked this pull request as ready for review September 9, 2022 15:52
@dac09 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
because *.d.ts don't get copied to dist
@@ -0,0 +1,3 @@
{
"extends": "../../tsconfig.json"
Copy link
Contributor

@mrazauskas mrazauskas Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
  },
}

Copy link
Collaborator Author

@dac09 dac09 Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.*"
    ]
}

Copy link
Contributor

@mrazauskas mrazauskas Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Add empty tsconfig.json to __typetests__ folder.
  2. Run yarn tsc -p src/__typetests__. All works.
  3. Add "compilerOptions": {"composite": true} to the same tsconfig.json and run the same command.
  4. Oh.. that’s the error we saw.. And it came from tsc this time.
  5. 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@dac09 dac09 Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator Author

@dac09 dac09 Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
{
"extends": "../../tsconfig.json"
Copy link
Contributor

@mrazauskas mrazauskas Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Add empty tsconfig.json to __typetests__ folder.
  2. Run yarn tsc -p src/__typetests__. All works.
  3. Add "compilerOptions": {"composite": true} to the same tsconfig.json and run the same command.
  4. Oh.. that’s the error we saw.. And it came from tsc this time.
  5. 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
Copy link
Member

Tobbe commented Sep 15, 2022

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

@Tobbe Tobbe enabled auto-merge (squash) September 16, 2022 12:18
@Tobbe Tobbe merged commit 22c3d1d into redwoodjs:main Sep 16, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Sep 16, 2022
@SimenB
Copy link

SimenB commented Sep 16, 2022

👏

@dac09
Copy link
Collaborator Author

dac09 commented Sep 16, 2022

Hello @SimenB good to see you around here :)

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

SimenB commented Sep 16, 2022

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
Copy link
Collaborator Author

dac09 commented Sep 16, 2022

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
Copy link
Contributor

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

Copy link
Collaborator Author

dac09 commented Sep 16, 2022

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) 👍👍👍👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Configure and add type-tests [Bug?]: Missing parameter for route when using glob pattern
5 participants