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

TS types for pages #51

Open
jfsiii opened this issue Aug 15, 2021 · 9 comments
Open

TS types for pages #51

jfsiii opened this issue Aug 15, 2021 · 9 comments

Comments

@jfsiii
Copy link

jfsiii commented Aug 15, 2021

Currently, (as far as I can tell), pages do not receive typed props

e.g. examples/react/src/pages/Home.jsx
Screen Shot 2021-08-15 at 7 28 03 PM

Ideally, a page would get the types automatically. Worst case, a page could import a type (from functions/{props,api} or elsewhere) but I believe it's really important the pages receive typed props.

Perhaps this is all under the "Rewrite in TypeScript" TODO, but I wanted to highlight this specific case and ask for it to be prioritized.

@jfsiii
Copy link
Author

jfsiii commented Aug 15, 2021

Let me know if I'm missed something and it is possible today. I'm happy to help, especially if you've already got an idea of how/where to do it.

It seems like step 1 is making the connection between the return type of EdgeProps['handler'] and a page, but even that still leaves us with any with the current types

Screen Shot 2021-08-15 at 7 50 15 PM

We'll also need a way to specify the type for ReturnedPropsPayload['data']. Maybe we'll get lucky and have a way the compiler knows the exact mapping from prop getter to page. Otherwise it's seems we'll define a type in one place and import/use it in props, pages, etc as needed.

Again, happy to help how I can. I'm pretty comfortable with TS but am brand new to Vite and this very exciting project.

@frandiox
Copy link
Owner

@jfsiii Hi!
The thing is, the props for each page comes from user-land since you can basically return anything you need from the props handler (that's why the data property is any).
Therefore, the only thing that comes to mind is having a place where you export your own types. Then, import the types in both your props-handler and your page.

I'm not that fluent in TS-compiler stuff so not sure if this can be automated in any way 🤔 Do you have any proposal?

@jfsiii
Copy link
Author

jfsiii commented Aug 24, 2021

The thing is, the props for each page comes from user-land since you can basically return anything you need from the props handler (that's why the data property is any).

@frandiox Right. But we can create a type for that response, or let the user supply it. There are a few options depending on the DX we want, but one option is to export the type handler returns and let it accept a Type (i.e. a Generic). e.g. with

// index.d.ts
export type ReturnedPropsPayload<T=any> = PropsOptions & { data?: T }

it can be used like

diff --git a/examples/react/functions/props/post.ts b/examples/react/functions/props/post.ts
index d664954..a2229bd 100644
--- a/examples/react/functions/props/post.ts
+++ b/examples/react/functions/props/post.ts
@@ -1,13 +1,21 @@
+import type { ReturnedPropsPayload } from 'vitedge';
 import { defineEdgeProps } from 'vitedge/define'

+export interface PropsData {
+  server: boolean,
+  params: Record<string, unknown>,
+}
+
+type EdgePropsData = ReturnedPropsPayload<PropsData>
+
 export default defineEdgeProps({
-  async handler({ params = {}, query = {} }) {
+  async handler({ params = {} }): Promise<EdgePropsData> {
     return {
       data: {
         server: true,
         params,
       },
    }
    };
   },
   options: {
     cache: {
and be type-safe (screenshots from VS Code) Screen Shot 2021-08-23 at 7 32 57 PM Screen Shot 2021-08-23 at 7 33 10 PM Screen Shot 2021-08-23 at 7 33 28 PM Screen Shot 2021-08-23 at 7 35 37 PM Screen Shot 2021-08-23 at 7 35 49 PM Screen Shot 2021-08-23 at 7 35 58 PM Screen Shot 2021-08-23 at 7 36 11 PM

That at least gives a typed props handler. There are similar options with the API endpoints and page handlers.

Ideally, vitedge would make the connections so the page handlers would know specifically which props they received (Home, About, etc) but, worst case, the user could take a similar approach as before

diff --git a/examples/react/src/pages/post/index.jsx b/examples/react/src/pages/post/index.jsx
index b63ca64..c0bb013 100644
--- a/examples/react/src/pages/post/index.jsx
+++ b/examples/react/src/pages/post/index.jsx
@@ -1,6 +1,14 @@
 import React from 'react'
+// type we defined/exported earlier
+import type { PropsData } from '../../../functions/props/post';

-const Post = ({ params = {}, isLoadingProps, isRevalidatingProps }) => {
+// comes from vitedge or somewhere outside page
+type PageProps = {
+  isLoadingProps: boolean
+  isRevalidatingProps: boolean
+};
+
+const Post = (props: PageProps & PropsData) => {
+  const { params, isLoadingProps, isRevalidatingProps, server,  } = props;
   if (!import.meta.env.SSR) {
     console.log({ isLoadingProps, isRevalidatingProps })
   }
Screenshots for page handler Screen Shot 2021-08-23 at 8 12 32 PM Screen Shot 2021-08-23 at 8 26 29 PM

Unfortunately, this is params is still Record<string, unknown> vs something route-specific like { postId: string }

TL;DR; I'm sure we can improve things; it's largely a matter of taste (desired DX) and some work defining the types

@jfsiii
Copy link
Author

jfsiii commented Aug 24, 2021

Also, if we define the routes a bit differently

diff showing individual routes with unique `name` and `path`
diff --git a/examples/react/src/routes.js b/examples/react/src/routes.js
index 81695be..d98e813 100644
--- a/examples/react/src/routes.js
+++ b/examples/react/src/routes.js
@@ -1,30 +1,43 @@
 import { createElement } from 'react'
 
-export default [
-  {
-    path: '/',
-    name: 'home',
-    exact: true,
-    component: () => import('./pages/Home'),
-  },
-  {
-    path: '/about',
-    name: 'about',
-    component: () => import('./pages/About'),
-  },
-  {
-    path: '/post/:postId',
-    name: 'post',
-    component: () => import('./pages/post'),
-    meta: {
-      propsGetter: 'post',
-    },
+export const HomeRoute = {
+  path: '/' as const,
+  name: 'home' as const,
+  exact: true as const,
+  component: () => import('./pages/Home'),
+};
+
+export const AboutRoute = {
+  path: '/about' as const,
+  name: 'about' as const,
+  component: () => import('./pages/About'),
+};
+
+export const GetPostByIdRoute = {
+  path: '/post/:postId' as const,
+  name: 'post' as const,
+  component: () => import('./pages/post'),
+  meta: {
+    propsGetter: 'post' as const,
   },
-].map(({ component: fn, ...route }) => {
+};
+
+
+export default [
+  HomeRoute,
+  AboutRoute,
+  GetPostByIdRoute,
+].map((route) => {
+  const { component: fn,  ...rest } = route;
   let component = null
   return {

TS can potentially tell exactly which route we're dealing with and the specific values it has (e.g. path: '/about' vs path: string) which would help connect the return types we're defining elsewhere

More screenshots Screen Shot 2021-08-23 at 8 53 36 PM Screen Shot 2021-08-23 at 8 55 01 PM Screen Shot 2021-08-23 at 8 58 51 PM Screen Shot 2021-08-23 at 9 02 02 PM Screen Shot 2021-08-23 at 9 02 11 PM Screen Shot 2021-08-23 at 9 02 34 PM

@frandiox
Copy link
Owner

@jfsiii Thanks for the thorough explanation! Yeah, I totally agree with providing generics, but perhaps it would be better at the wrapper level?

declare module 'vitedge/define' {
  export const defineApiEndpoint: <T = any>(apiEndpoint: ApiEndpoint<T>) => ApiEndpoint<T>
  export const defineEdgeProps: <T = any>(edgeProps: EdgeProps<T>) => EdgeProps<T>
}

Or perhaps with 2 generics, one for the returned data type and another one for the params?

The hardest part I see is the "automatic connection" between frontend and backend. In many cases the routes are created automatically using plugins (such as vite-plugin-pages), but even if we define routes manually with const, I'm not really sure how to take that to the backend 🤔 . Of course, any ideas or exploration in this area is very welcome!

As you say, worst case scenario the user can import the types manually. Maybe we could provide some aliases automatically so they can do import type { MyProps } from '@props/my-page easily?

@frandiox
Copy link
Owner

Some crazy idea that probably won't work with TS: create a virtual entry file that imports every prop handler type and re-exports the returned data type. Roughly this, not sure if this is even correct syntax:

import type { default as AboutHandler } from '..../functions/props/about'
export type AboutProps = ReturnType<AboutHandler['handler']>['data'] // This should also unwrap the Promise type
// About.jsx
import type { AboutProps } from 'virtual:vitedge-props'

Not even sure if this is desirable, just brainstorming 😅

@jfsiii
Copy link
Author

jfsiii commented Aug 27, 2021

I hadn't thought about the virtual entry (I'm new to vite/vue/etc and they're new to me) but that seems really interesting.

In used them in https://github.com/jfsiii/reactesse-use-deep-props/blob/5f8420c6828ef2502dac362e299d211aaa8793e3/vite.config.ts#L49 toI stash the original path the the page file (e.g. ./src/pages/hi/[name].tsx) route.meta.pageFilePath. It seems like it could be really useful later.

@jfsiii
Copy link
Author

jfsiii commented Aug 27, 2021

I'm not sure how far we can get to knowing exactly what type are on what page/props/api, but I would like to start with getting types for pages, like there are for EdgeProps and ApiEndpoint where there are some hints about what's passed.

I might be missing something, but I was adding console.logs to see what values were present because I couldn't get type info

Screen Shot 2021-08-26 at 10 59 28 AM

I've done some hacking on them locally and am happy to contribute, but I wasn't sure how to start & iterate. I think that requires some changes in vitedge/index.d.ts & maybe exporting some things from vitedge/react like vite-ssr does, but wasn't sure if you already had a vision for it. I guess I can (Draft) PR and we could discuss?

WDYT about an issue for the "Rewrite in TypeScript" TODO? I'm mostly curious if you imagined a big-bang "rename everything .ts" or wanted to keep with the shadowed *.d.ts files. I'm very comfortable with types but I've only worked in an all TS setup, not .d.ts or the jsdoc @type { import('...').Type } workflow.

@frandiox
Copy link
Owner

frandiox commented Aug 27, 2021

@jfsiii I want to turn everything to .ts files, just like we did in vite-ssr. The main src/index.d.ts needs to stay there because that one are "virtual types". It basically merges types for React and Vue when you use import vitedge from 'vitedge' instead of import vitedge from 'vitedge/react'.

The props fetching works different in React and Vue. isRevalidatingProps and isLoadingProps, for example, exist only in React. Perhaps we should turn the project to TS before implementing this but, if you know a way, feel free to open a PR and we can discuss further there.
The wrapper App should receive this Context type here, but I think we are not re-exporting it in Vitedge from vite-ssr.
You can see the props passed to React pages here (it's basically the 2 booleans and the state). The ...rest parameter should be anything the user passes to the page manually (e.g. <router.component exact={true} hello={'world'} />). The fact that you can see location, match, etc. might be due to react-dom-config's renderRoutes is passing that 🤔

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

No branches or pull requests

2 participants