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

feat: infer types for waku router #854

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

tylersayshi
Copy link
Contributor

@tylersayshi tylersayshi commented Aug 28, 2024

Currently this changeset gets createPages ready to return an inferred type for each response of createPage

TODO:

  • Create a union type of all possible paths given the pages
  • Pass this union to an override-able interface so waku projects can consume the pages type safety (same as tanstack router's approach)
  • Use that type in our <Link /> component
  • Update tests and refactor examples to match new api

Copy link

vercel bot commented Aug 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
waku ✅ Ready (Inspect) Visit Preview Sep 13, 2024 1:50am

@tylersayshi
Copy link
Contributor Author

@dai-shi & @sandren I just want to mention that while the enforcement of returning the pages here would mean a slight breaking change to createPages, it would unlock theoretically typing anything that is statically defined in each call to createPage

Right now, I am just inferring path, but I intend to follow this up with inferring the full Page object to get things like the staticPaths and render. If we ever wanted to look into things like typesafe query params, this would be an interesting place to add those features in without any further changes to the external api because we have setup the api for returning the static pages which makes them available for inference.

@tylersayshi
Copy link
Contributor Author

@dai-shi How would you feel about turning noErrorTruncation on in one of the tsconfigs?

I am currently setting this to true locally while developing then leaving it out of my commits, but here is why it is nice for me:

This is without setting noErrorTruncation:

image

With adding "noErrorTruncation": true,:

image

@tylersayshi
Copy link
Contributor Author

Another update here:

image

All possible paths are being inferred from looking at path, render, and staticPaths on each page response 🥳

@tylersayshi
Copy link
Contributor Author

tylersayshi commented Sep 7, 2024

Another update here:

...

All possible paths are being inferred from looking at path, render, and staticPaths on each page response 🥳

Just realized that the above screenshot does not handle the catch-all/wildcard static slugs correctly yet, so I fixed that here:

image

Copy link

codesandbox-ci bot commented Sep 8, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@tylersayshi
Copy link
Contributor Author

tylersayshi commented Sep 8, 2024

It's working in the router example now... I plan to revert that example to install from a published node version, but am leaving it in there in case @sandren or @dai-shi want to play around with this in that working example project :)

image

dai-shi pushed a commit that referenced this pull request Sep 8, 2024
This results in the ts preview of vscode not truncating the hovered
information of a type or error. Helpful for inspecting long types.

Follow up to:
#854 (comment)

Co-authored-by: Tyler <[email protected]>
@tylersayshi tylersayshi marked this pull request as ready for review September 8, 2024 03:30
Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

some comments:

examples/07_router/src/entries.tsx Outdated Show resolved Hide resolved
packages/waku/src/router/create-pages.ts Outdated Show resolved Hide resolved
packages/waku/src/router/index.ts Outdated Show resolved Hide resolved
Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

some more comments 😄

packages/waku/package.json Show resolved Hide resolved
packages/waku/src/router/base-types.ts Outdated Show resolved Hide resolved
packages/waku/src/router/base-types.ts Show resolved Hide resolved
packages/waku/src/router/base-types.ts Outdated Show resolved Hide resolved
packages/waku/src/router/inferred-router-types.ts Outdated Show resolved Hide resolved
Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

hope it makes sense.

packages/waku/src/router/client.ts Outdated Show resolved Hide resolved
packages/waku/src/router/server.ts Outdated Show resolved Hide resolved
docs/create-pages.mdx Outdated Show resolved Hide resolved
examples/07_router/src/entries.tsx Outdated Show resolved Hide resolved
Comment on lines +6 to +7
// This needs type annotations for the return type of createPages
// @see https://github.com/microsoft/TypeScript/issues/42873#issuecomment-2065572017
Copy link
Owner

Choose a reason for hiding this comment

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

I was curious about this explicit annotation in other repo.
Does the linked issue commend suggest the explicit annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes... it is apparently a known issue with type inference inside pnpm projects with symlinked packages.

After visiting the discord for arktype I was linked to that issue on the Typescript repo that I left in the comments. Here is the summary that I ended up putting on the arktype discord after a chat with their author:

image

discord link if you'd like to read the full thread

arktype discord if you need a join link

@tylersayshi
Copy link
Contributor Author

hope it makes sense.

Yep! all makes sense and were simple changes I think. 215e6e2

@tylersayshi
Copy link
Contributor Author

@dai-shi some idea names for base-types.ts now that it only has the empty interface to be overridden externally:

  • external.ts or external-types.ts
  • router-types.ts
  • public-types.ts

Also, are there other changes you'd like to see with this PR?

@dai-shi
Copy link
Owner

dai-shi commented Sep 10, 2024

I'll do a full review later in this week.

Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM. Note that I didn't read the code in detail.

packages/waku/src/router/client.ts Outdated Show resolved Hide resolved
@tylersayshi
Copy link
Contributor Author

Otherwise, LGTM. Note that I didn't read the code in detail.

Sounds good 👍 - squashed and rebased. Should be good to merge 😄

@dai-shi dai-shi merged commit a446b6e into dai-shi:main Sep 13, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants