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
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
3330c73
Move route parser to inside rwjsRouter
dac09 Sep 6, 2022
09e462e
Simple tsdlite setup with jest
dac09 Sep 7, 2022
6c86058
Add tsd tests for routeParamsTypes
dac09 Sep 7, 2022
73d5906
Don't use caret
dac09 Sep 8, 2022
b98bba6
Dedupe yarn
dac09 Sep 8, 2022
db82751
Merge branch 'main' of github.com:redwoodjs/redwood into try/tsd-tests
dac09 Sep 9, 2022
7ca4f78
Break up RouteParser, add aditional tests
dac09 Sep 9, 2022
fbd69a0
Fix glob type |
dac09 Sep 9, 2022
e671ab2
Comment
dac09 Sep 9, 2022
d870521
Merge branch 'main' into try/tsd-tests
dac09 Sep 9, 2022
684f7c0
Rename one of the types
dac09 Sep 9, 2022
806d120
Merge main
dac09 Sep 12, 2022
50a7671
Use a ts file for types instead,
dac09 Sep 12, 2022
1845e62
Update packages/router/src/routeParamsTypes.ts
dac09 Sep 12, 2022
6fdeae5
Add extra tsconfig to typetests |
dac09 Sep 12, 2022
88edc39
Update packages/router/src/__typetests__/routeParamsTypes.test.ts
dac09 Sep 12, 2022
d860055
Merge branch 'main' into try/tsd-tests
dac09 Sep 13, 2022
061bcc8
Add a couple of more router matchPath tests
Tobbe Sep 15, 2022
8706a05
Merge branch 'main' into try/tsd-tests
dac09 Sep 15, 2022
e68fb7f
Add more tests
dac09 Sep 15, 2022
64758f6
Fix the strange edge cases
dac09 Sep 15, 2022
c1d424f
Use expectType in a different way for more accurate tests
dac09 Sep 15, 2022
9e3f586
Add FAQ to tests
dac09 Sep 15, 2022
37e1c79
tweak messages
dac09 Sep 15, 2022
7359f8c
Merge branch 'main' into try/tsd-tests
Tobbe Sep 16, 2022
b80f2d2
Merge branch 'main' into try/tsd-tests
dac09 Sep 16, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"@testing-library/react": "12.1.5",
"@testing-library/react-hooks": "8.0.1",
"@testing-library/user-event": "14.4.3",
"@tsd/typescript": "4.8.2",
"@types/babel__generator": "7.6.4",
"@types/fs-extra": "9.0.13",
"@types/jest": "29.0.1",
Expand All @@ -81,6 +82,7 @@
"fs-extra": "10.1.0",
"is-port-reachable": "3.1.0",
"jest": "29.0.3",
"jest-runner-tsd": "4.0.0",
"jscodeshift": "0.13.1",
"lerna": "5.4.3",
"lodash.template": "4.5.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
import '@redwoodjs/router'
import { RouteParams, QueryParams } from '@redwoodjs/router'
import { A } from 'ts-toolbelt'



type RouteParams<Route> = ${"Route extends `${string}/${infer Rest}`"}
? A.Compute<ParsedParams<Rest>>
: {}

type QueryParams = Record<string | number, string | number | boolean>

declare module '@redwoodjs/router' {
interface AvailableRoutes {
// Only "<Route />" components with a "name" and "path" prop will be populated here.
Expand All @@ -20,39 +13,3 @@ ${routes.map(
}
}

type ParamType<match> = match extends 'Int'
? number
: match extends 'Boolean'
? boolean
: match extends 'Float'
? number
: string

// Path string parser for Redwood Routes
type ParsedParams<PartialRoute> =
// {a:Int}/[...moar]
${"PartialRoute extends `{${infer Param}:${infer Match}}/${infer Rest}`"}
? // check for greedy match e.g. {b}/{c:Int}
// Param = b}/{c, Rest2 = {c, Match = Int so we reconstruct the old one {c + : + Int + }
${"Param extends `${infer Param2}}/${infer Rest2}`"}
? { [ParamName in Param2]: string } &
${"ParsedParams<`${Rest2}:${Match}}`> &"}
${"ParsedParams<`${Rest}`>"}
${": { [Entry in Param]: ParamType<Match> } & ParsedParams<`${Rest}`>"}
: // has type, but at the end e.g. {d:Int}
${"PartialRoute extends `{${infer Param}:${infer Match}}`"}
? // Greedy match order 2
${"Param extends `${infer Param2}}/${infer Rest2}`"}
? { [ParamName in Param2]: string } &
${"ParsedParams<`${Rest2}:${Match}}`>"}
: { [Entry in Param]: ParamType<Match> }
: // no type, but has stuff after it, e.g. {c}/{d}
${"PartialRoute extends `{${infer Param}}/${infer Rest}`"}
${"? { [ParamName in Param]: string } & ParsedParams<`${Rest}`>"}
: // last one with no type e.g. {d}
${"PartialRoute extends `{${infer Param}}`"}
? { [ParamName in Param]: string }
: // if theres a non param
${"PartialRoute extends `${string}/${infer Rest}`"}
${"? ParsedParams<`${Rest}`>"}
: {}
18 changes: 16 additions & 2 deletions packages/router/jest.config.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,19 @@
/** @type {import('@jest/types').Config.InitialOptions} */
module.exports = {
setupFilesAfterEnv: ['./jest.setup.js'],
testEnvironment: 'jest-environment-jsdom',
projects: [
{
displayName: 'code',
setupFilesAfterEnv: ['./jest.setup.js'],
testEnvironment: 'jest-environment-jsdom',
testMatch: ['**/*.test.+(ts|tsx|js)', '!**/__typetests__/*.ts'],
},
{
displayName: {
color: 'blue',
name: 'types',
},
runner: 'jest-runner-tsd',
testMatch: ['**/__typetests__/*.test.ts'],
},
],
}
2 changes: 1 addition & 1 deletion packages/router/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"build:types": "tsc --build --verbose",
"build:watch": "nodemon --watch src --ext \"js,ts,tsx\" --ignore dist --exec \"yarn build\"",
"prepublishOnly": "NODE_ENV=production yarn build",
"test": "jest src",
"test": "jest",
"test:watch": "yarn test --watch"
},
"dependencies": {
Expand Down
105 changes: 105 additions & 0 deletions packages/router/src/__typetests__/routeParamsTypes.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import { expectAssignable } from 'tsd-lite'

// @WARN!: I'm importing this from the built package.
// So you will need to build, before running test again!
// See https://github.com/jest-community/jest-runner-tsd/issues/111
dac09 marked this conversation as resolved.
Show resolved Hide resolved
import type { RouteParams, ParamType } from '@redwoodjs/router'

describe('RouteParams<>', () => {
test('Single parameters', () => {
expectAssignable<RouteParams<'bazinga/{id:Int}'>>({
id: 1,
})
})

test('Route string with no types defaults to string', () => {
expectAssignable<RouteParams<'/blog/{year}/{month}/{day}/{slug}'>>({
year: '2020',
month: '01',
day: '01',
slug: 'hello-world',
})
})

test('Custom param types', () => {
const customParams = {
name: 'hello-world-slug',
}

expectAssignable<RouteParams<'/post/{name:slug}'>>(customParams)
})

test('Multiple Glob route params', () => {
const globRoutes = {
fromDate: '2021/11/03',
toDate: '2021/11/17',
}

expectAssignable<RouteParams<'/from/{fromDate...}/to/{toDate...}'>>(
globRoutes
)
})

test('Single Glob route params', () => {
const globRoutes = {
fromDate: '2021/11/03',
}

expectAssignable<RouteParams<'/from/{fromDate...}'>>(globRoutes)
})

test('Glob params in the middle', () => {
test('Multiple Glob route params', () => {
const middleGlob = {
folders: 'src/lib/auth.js',
}

expectAssignable<RouteParams<'/repo/{folders...}/edit'>>(middleGlob)
})
})

test('Mixed typed and untyped params', () => {
const untypedFirst = {
b: 'bazinga',
c: true,
}

const typedFirst = {
b: 1245,
c: 'stringy-string',
}

expectAssignable<RouteParams<'/mixed/{b}/{c:Boolean}'>>(untypedFirst)
expectAssignable<RouteParams<'/mixed/{b:Float}/{c}'>>(typedFirst)
})

test('Params in the middle', () => {
const paramsInTheMiddle = {
authorId: 'id:author',
id: 10,
}

expectAssignable<RouteParams<'/posts/{authorId:string}/{id:Int}/edit'>>(
paramsInTheMiddle
)
})
})

describe('ParamType<>', () => {
test('Float', () => {
expectAssignable<ParamType<'Float'>>(1.02)
})

test('Boolean', () => {
expectAssignable<ParamType<'Boolean'>>(true)
expectAssignable<ParamType<'Boolean'>>(false)
})

test('Int', () => {
expectAssignable<ParamType<'Int'>>(51)
})

test('String', () => {
expectAssignable<ParamType<'String'>>('bazinga')
})
})
3 changes: 3 additions & 0 deletions packages/router/src/__typetests__/tsconfig.json
Original file line number Diff line number Diff line change
@@ -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.

}
3 changes: 3 additions & 0 deletions packages/router/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,6 @@ export interface AvailableRoutes {
}

export { SkipNavLink, SkipNavContent } from '@reach/skip-nav'

// Used by packages/internal/src/generate/templates/web-routerRoutes.d.ts.template
export * from './routeParamsTypes'
114 changes: 114 additions & 0 deletions packages/router/src/routeParamsTypes.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import { A } from 'ts-toolbelt'

type GenericParams = Record<string | number, string | number | boolean>

export type QueryParams = GenericParams

export type RouteParams<Route> = Route extends `${string}/${infer Rest}`
? A.Compute<ParsedParams<Rest>>
: GenericParams

export type ParamType<match> = match extends 'Int'
? number
: match extends 'Boolean'
? boolean
: match extends 'Float'
? number
: string

// This is used for a specific case where the first param
// doesnt have a type, but second one does
// See comment above it's usage
type ParamsFromGreedyMatch<
TParam extends string,
TMatch extends string,
TRest extends string
> = {
[ParamName in TParam as RemoveGlobDots<ParamName>]: string
} & ParsedParams<`${TRest}:${TMatch}}`> &
ParsedParams<`${TRest}`>

// Note that this has to be first in the list, because it does greedy param checks
type TypedParamInFront<
TParam extends string,
TMatch extends string,
TRest extends string
> = TParam extends `${infer Param2}}/${infer Rest2}`
? // check for greedy match (basically if the param contains a slash in it)
// e.g. in {b}/{c:Int} it matches b}/{c as the param
// Rest2 = {c, Match = Int so we reconstruct the old one {c + : + Int + }
ParamsFromGreedyMatch<Param2, TMatch, Rest2>
: // Otherwise its a regular match
{
[ParamName in TParam]: ParamType<TMatch>
} & ParsedParams<`${TRest}`>

// This is the second part of greedy match
// has type, but at the end e.g. {d:Int} or d:Int} <-- no opening brace
// Needs to be right after TypedParamInFront
type TypedParamAtEnd<
TParam extends string,
TMatch extends string
> = TParam extends `${infer Param2}}/${infer Rest2}`
? {
[ParamName in Param2]: string
} & ParsedParams<`${Rest2}:${TMatch}}`>
: { [ParamName in TParam]: ParamType<TMatch> }

// This mapper takes a param name and will remove dots if its a glob
// e.g. fromDate... -> fromDate
// Only used when the param doesn't have a type, because glob params dont have types
type RemoveGlobDots<Param> = Param extends `${infer GlobParamName}...`
? GlobParamName
: Param

// no type, but has stuff after it, e.g. {c}/{d} or {c}/bazinga
type MultiParamsWithoutType<TParam extends string, TRest extends string> = {
[ParamName in TParam as RemoveGlobDots<ParamName>]: string
} & ParsedParams<`${TRest}`>

type JustParamNoType<TParam extends string> = {
[ParamName in TParam as RemoveGlobDots<ParamName>]: string
}

// Path string parser for Redwood Routes
type ParsedParams<PartialRoute> =
// PartialRoute extends `{${infer GlobParam}...}/${infer Rest}}`
// ? ParsedParams<GlobParam> & ParsedParams<Rest>
// : // {a:Int}/[...moar]
PartialRoute extends `{${infer Param}:${infer Match}}/${infer Rest}`
? TypedParamInFront<Param, Match, Rest>
: // has type, but at the end e.g. {d:Int}
PartialRoute extends `{${infer Param}:${infer Match}}`
? // Greedy match order 2
TypedParamAtEnd<Param, Match>
: // no type, but has stuff after it, e.g. {c}/{d} or {c}/bazinga
PartialRoute extends `{${infer Param}}/${infer Rest}`
? MultiParamsWithoutType<Param, Rest>
: // last one with no type e.g. {d} - just a param
PartialRoute extends `{${infer Param}}`
? JustParamNoType<Param>
: // if theres a non param
PartialRoute extends `${string}/${infer Rest}`
? ParsedParams<`${Rest}`>
: // Fallback when doesn't match any of these
Record<string | number, any>

/**
* Translation in pseudocode without ternaries
*
if ('{c:Int}/...rest') {
checkForGreedyMatch()
} else if ('{c:Int}') {
typedParamAtEnd()
} else if ('{c}/...rest') {
multipleParamsNoTypes()
} else if('{d}') {
justParamNoType()
} else if ('bazinga/..rest') {
// Call itself
parseParamsRecursiveCall(rest)
}


**/
Loading