qual: make config hook types more precise #1137

lourot · 2023-09-21T18:17:07Z

@brillout, validated with tsc --noEmit on the example. Is this going in the right direction? If yes I would like to:

do the same for other hooks
add types to all such hooks in all existing typescript examples.

Is it OK with you?

lourot · 2023-09-21T19:10:19Z

@brillout I think it is better now

vite-plugin-ssr/shared/page-configs/Config.ts

lourot · 2023-09-27T14:05:56Z

@brillout does it now look more like what you had in mind? Thanks

lourot · 2023-09-27T15:07:57Z

examples/auth $ pnpm exec tsc --noEmit
pages/+config.h.ts:8:3 - error TS2322: Type '{ onRenderHtml: "import:vike-react/renderer/onRenderHtml"; onRenderClient: "import:vike-react/renderer/onRenderClient"; passToClient: string[]; clientRouting: true; hydrationCanBeAborted: true; meta: { ...; }; }' is not assignable to type 'Config | `import:${string}` | Config[] | `import:${string}`[] | undefined'.
  Type '{ onRenderHtml: "import:vike-react/renderer/onRenderHtml"; onRenderClient: "import:vike-react/renderer/onRenderClient"; passToClient: string[]; clientRouting: true; hydrationCanBeAborted: true; meta: { ...; }; }' is not assignable to type 'ConfigBuiltIn & Config & ConfigVikeSvelte'.
    Type '{ onRenderHtml: "import:vike-react/renderer/onRenderHtml"; onRenderClient: "import:vike-react/renderer/onRenderClient"; passToClient: string[]; clientRouting: true; hydrationCanBeAborted: true; meta: { ...; }; }' is not assignable to type 'ConfigBuiltIn'.
      Types of property 'meta' are incompatible.
        Type '{ Head: { env: "server-only"; }; Layout: { env: "server-and-client"; }; title: { env: "server-and-client"; }; description: { env: "server-only"; }; favicon: { env: "server-only"; }; lang: { env: "server-only"; }; ssr: { ...; }; }' is not assignable to type '`import:${string}` | ConfigMeta | undefined'.
          Type '{ Head: { env: "server-only"; }; Layout: { env: "server-and-client"; }; title: { env: "server-and-client"; }; description: { env: "server-only"; }; favicon: { env: "server-only"; }; lang: { env: "server-only"; }; ssr: { ...; }; }' is not assignable to type 'ConfigMeta'.
            Property 'ssr' is incompatible with index signature.
              Type '{ env: "config-only"; effect: ConfigEffect; }' is not assignable to type 'ConfigDefinition'.
                The types returned by 'effect(...)' are incompatible between these types.
                  Type 'import("/home/lourot/Documents/git/vike/vite-plugin-ssr/node_modules/.pnpm/vite-plugin-ssr@0.4.142_vite@4.3.9/node_modules/vite-plugin-ssr/dist/esm/shared/page-configs/Config").Config | undefined' is not assignable to type 'import("/home/lourot/Documents/git/vike/vite-plugin-ssr/vike/dist/esm/shared/page-configs/Config").Config | undefined'.
                    Type 'ConfigBuiltIn & Config & ConfigVikeReact' is not assignable to type 'Config | undefined'.
                      Type 'import("/home/lourot/Documents/git/vike/vite-plugin-ssr/node_modules/.pnpm/vite-plugin-ssr@0.4.142_vite@4.3.9/node_modules/vite-plugin-ssr/dist/esm/shared/page-configs/Config").ConfigBuiltIn & Vike.Config & VikePackages.ConfigVikeReact' is not assignable to type 'import("/home/lourot/Documents/git/vike/vite-plugin-ssr/vike/dist/esm/shared/page-configs/Config").ConfigBuiltIn & Vike.Config & VikePackages.ConfigVikeReact'.
                        Type 'ConfigBuiltIn & Config & ConfigVikeReact' is not assignable to type 'ConfigBuiltIn'.
                          Types of property 'extends' are incompatible.
                            Type 'Config | `import:${string}` | `import:${string}`[] | Config[] | undefined' is not assignable to type 'Config | `import:${string}` | Config[] | `import:${string}`[] | undefined'.
                              Type 'ConfigBuiltIn & Config & ConfigVikeReact' is not assignable to type 'Config | `import:${string}` | Config[] | `import:${string}`[] | undefined'.

I'm pretty sure this is because the vike auth example +config.h.ts includes the new vike Config that I have touched, but also wants to satisty vike-react's Config, which wants to satisfy the old vike Config. In other words there is a kind of circular type dep now accross vike and vike-react.

@brillout I believe we should tackle this circular dep first as this will hit us back whenever we touch the Config type. What do you think? Does that make sense to you if I start looking into this?

brillout · 2023-09-27T15:14:17Z

Yes, exactly.

If the user always extends PageContext{Client,Server} by using Vike.PageContext then I'm thinking we can even remove the generic type.

Btw. for onBeforeRender() specifically, since it's run on the server-side only, we should use PageContextServer instead of PageContext.

An interesting question is what happens when the user sets onBeforeRender's env to server-and-client? So far, I believe the only solution would be to code-gen types.

brillout · 2023-09-27T15:27:12Z

As for the TS error: I'm inclined to think it's a mismatch between the Config used by vike-react and the Config you're modifying. I'd check wether vike-react uses its own copy of vike/Config or whether it uses the one you're modifying.

We set this in the root package.json:

  "pnpm": {
    "overrides": {
      "vike": "link:./vike/"
    },

But, if I remember correctly, this isn't recursive.

brillout · 2023-09-27T15:31:17Z

Actually, since vike is a peer dependency, it should resolve to the same/local vike package 🤔.

lourot · 2023-09-27T15:35:17Z

If the user always extends PageContext{Client,Server} by using Vike.PageContext then I'm thinking we can even remove the generic type.

You made me realize I wanted to use Vike.PageContext and not PageContext. I mixed up. I'll fix that and see what happens if I remove the generic.

Regarding "vike": "link:./vike/", we don't write that in the examples. Probably because we want the examples to work with npm?

Alternatively I was thinking of removing statisfies Config when using extends: vikeReact. Rationale:

// examples/auth/pages/+config.h.ts

import Layout from '../layouts/LayoutDefault'
import vikeReact from 'vike-react'

export default {
  Layout,
  passToClient: ['userFullName'],
  extends: vikeReact
} // `satisfies Config` not needed, as already implied by `extends vikeReact`, which is an object
  // that already states explicitly `satisfies Config`

What do you think?

Actually, since vike is a peer dependency, it should resolve to the same/local vike package 🤔.

In this example actually vike is also a normal dependency. Maybe it shouldn't be. Let me play with that first. Thanks for all the pointers :)

brillout · 2023-09-27T15:35:23Z

Does that make sense to you if I start looking into this?

Yes. (I had a quick look and it does seem like pnpm is linking everything correctly, so I'm not sure why we get that TypeScript error.)

brillout · 2023-09-28T05:49:16Z

Probably because we want the examples to work with npm?

Yes exactly. That's why we use pnpm's overrides feature instead.

I was thinking of removing statisfies Config when using extends: vikeReact

I believe this wouldn't type passToClient nor Layout; it would only type vikeReact.

In this example actually vike is also a normal dependency.

The example itself declares a direct dependency to vike which is what I would expect. What I meant is that vike-react shouldn't declare a direct dependency to vike and its dependency to vike should be a peer dependency instead, which is actually already the case (I just double checked). So I believe vike-react and the example are doing things correctly. And since pnpm seems to link everything correctly, I wonder why that error occurs.

Let me know if you want me to dig into that TS error.

lourot · 2023-09-28T10:07:02Z

@brillout I found the culprit: the example has a dependency to an old vike-react (0.2.0), which still has a dependency to vite-plugin-ssr instead of vike, so it doesn't benefit from "vike": "link:./vike/". Switching to vike-react@0.3.0 solves the issue.

Thanks for the useful pointers. Updating the PR

brillout · 2023-09-28T11:07:25Z

Ah, indeed, I forgot to update that one. Thanks 🙏.

vike/shared/page-configs/Config.ts

examples/react-full-v1/renderer/+onRenderHtml.tsx

examples/path-aliases-v1/renderer/onRenderHtml.tsx

brillout · 2023-10-12T13:38:05Z

👍 I'm seeing you added the type utils for the hooks 💯.

Async route is actually deprecated, so the default should be sync. ~~I also think the default for onPageTransition{Start,End} hooks should be sync~~ onPageTransition{Start,End} hooks don't return any value so I think we may not need any (a)sync variant. Same for onHydrationEnd.

I'm thinking whethter other hooks should be sync by default. E.g. I expect most guard() hooks to be sync. Alternative: we always use async guard() hooks in the docs and examples. Any thoughts on this? (I'm thinking it could be nice to always use async hook when a hook can be async: it's then obivious that the hook can be async.)

brillout · 2023-10-12T13:49:52Z

I just tried and this works:

type OnHydrationEnd = (pageContext: PageContextClient) => Promise<void | undefined> | void | undefined;

So we indeed can remove the variants for hooks that return void | Promise<void>.

On the other hand, it's kinda nice to keep the convention HookName{Sync} consistent across all hooks.

Hm 🤔

lourot · 2023-10-12T14:05:08Z

Async route is actually deprecated, so the default should be sync. ~~I also think the default for onPageTransition{Start,End} hooks should be sync~~ onPageTransition{Start,End} hooks don't return any value so I think we may not need any (a)sync variant. Same for onHydrationEnd.

Make sense, yes

I'm thinking it could be nice to always use async hook when a hook can be async: it's then obivious that the hook can be async.

Yes I like this

type OnHydrationEnd = (pageContext: PageContextClient) => Promise<void | undefined> | void | undefined;

The problem with this is that we can't use ReturnType<OnHydrationEnd> anymore:

// error TS1064: The return type of an async function or method must be the global Promise<T> type. Did you mean to write 'Promise<void>'?
const onHydrationEnd: OnHydrationEnd = async (): ReturnType<OnHydrationEnd> => {
  console.log('Hydration finished; page is now interactive.')
}

This is why we stopped mixing Promise<something> | something in return-types. We could say it's OK not to support ReturnType<HookThatReturnsVoid> but I would prefer just providing the async variant instead.

lourot · 2023-10-12T14:37:19Z

examples/vue-full-v1/renderer/+config.h.ts

@@ -6,7 +6,7 @@ export default {
  passToClient: ['pageProps', 'title'],
  clientRouting: true,
  prefetchStaticAssets: 'viewport',
-  onHydrationEnd,
+  onHydrationEnd, // NOTE(aurelien): this assignment breaks if we stop supporting the sync variant


See inline comment. It seems like we're introducing a breaking change by not supporting the sync variant of void-returning hooks. So I think we shouldn't do that either.

My suggestion is that we keep OnHydrationEnd | OnHydrationEndSync, yet from now on, as you said, we try to make examples use async variants as much as we can. What do you think?

brillout · 2023-10-12T15:21:36Z

Ah, you're right, The return type of an async function or method must be the global Promise<T> type was the issue and I just checked: it's also problematic even for a return type void.

Ok let's go with your with your suggestion. In other words, looking at the list:

  Guard,
  GuardSync,
  OnBeforePrerenderStart,
  OnBeforePrerenderStartSync,
  OnBeforeRender,
  OnBeforeRenderSync,
  OnBeforeRoute,
  OnBeforeRouteSync,
  OnHydrationEnd,
  OnPageTransitionEnd,
  OnPageTransitionStart,
  OnPrerenderStart,
  OnPrerenderStartSync,
  OnRenderClient,
  OnRenderClientSync,
  OnRenderHtml,
  OnRenderHtmlSync,
  Route

Only Route should be sync by default (i.e. Route | RouteAsync). All other hooks should be HookName | HookNameSync (async by default).

we try to make examples use async variants as much as we can. What do you think?

Agreed. I think it's easier for the user if we keep things consistent.

brillout · 2023-10-12T15:27:19Z

Idea: we provide OnBeforeRenderAsync | OnBeforeRenderSync instead of OnBeforeRender | OnBeforeRenderSync. More verbose but clearer.

I think I've a slight preference for that. Thoughts?

Let me think this a bit.

lourot · 2023-10-12T15:29:02Z

Only Route should be sync by default (i.e. Route | RouteAsync). All other hooks should be HookName | HookNameSync (async by default).

Actually, there is a caveat I think. When browsing Vike's source code, I came to the conclusion that the following hooks are sync-only: route, onPageTransitionStart, onPageTransitionEnd. The reason is that there is no await being done in Vike's source code for exactly these three hooks, so I think there shouldn't be an async variant for them. Or am I mistaken? I can dig that up again if needed.

lourot · 2023-10-12T15:30:25Z

I think I've a slight preference for that.

Yes, I prefer explicit Async/Sync as well

brillout · 2023-10-12T15:30:42Z

The route definitely can be async (its async handling may not be obvious).

Let me check about onPageTransition*.

brillout · 2023-10-12T15:31:57Z

Yes, I prefer explicit Async/Sync as well

👍 Ok let's move with that then: we can add an alias type OnBeforeRender = OnBeforeRenderAsync later on anyways 👌.

brillout · 2023-10-12T15:35:01Z

Let me check about onPageTransition*.

Fixed: 58f822b.

brillout · 2023-10-12T15:37:11Z

Yes, I prefer explicit Async/Sync as well

Since we're explicit, I think we don't need to use async hooks in all examples. Some hooks are trivial and obviously sync: I think it's fine to use the *Sync type for them.

…ooks" This reverts commit 5b4e15c.

lourot · 2023-10-12T16:28:31Z

@brillout I think it looks good now :)

…mented Vike.PageContext on the boierlplates. But we can also add add a note if you want; as you wish.

…on in the future as we'll incrinsingly promote the vike-{react,vue,solid} packages). When the user uses Client Routing then the route functions are called on the server-side as well as on the client-side

brillout · 2023-10-12T17:18:27Z

A real beauty 😍

I've added a couple of commits (see the entire commit messages); let me know if you object. (I'm actually not 100% sure about my type Route = RouteSync commit but it feels weird to offer a RouteSync if RouteAsync is deprecated. Not sure. I guess we can keep my commit and experiment how we feel about it later on (we can always deprecated Route in favor of RouteSync).)

I'm 👍 for proceeding. Btw. we can already merge; we can add docs and update examples in an extra PR. Whatever works best for you.

lourot · 2023-10-12T17:36:59Z

vike/shared/page-configs/Config.ts

+ *
+ *  https://vike.dev/route
+ */
+type RouteSync = (pageContext: PageContextServer | PageContextClient) => { routeParams: Record<string, string> }


This won't work unfortunately, we had this topic earlier in this PR: PageContextServer won't be assignable to PageContextServer | PageContextClient. It works the other way around. In other words:

(A | B) => void is assignable to (A) => void, but

(A) => void is not assignable to (A | B) => void

So the user won't be able to stop using client routing and create a route() that takes only PageContextServer as argument. I can create a TS playground to illustrate if needed.

Yes I know. My idea is to tell Server Routing users to not use the Route type. We can increasingly assume users to use Client Routing because of vike-{react,vue,solid}.

My thinking is: users who use vike-{react,vue,solid} are more likely to be junior-ish and it's particularly important for them to provide friendly types.

Current users who use Server Routing are "expert users"; I think it's fine to be less friendly. (Alternatively we can provide RouteWithServerRouting, like PageContextClientWithServerRouting.)

In a nutshell: we highly polish things for Client Routing and neglect Server Routing. (Honestly, I'm even considering deprecating Server Routing.)

@brillout even an expert won't be able to perform such an assignment if they do it in +config.h.ts, similarly to the problem I was showing here: #1137 (comment)

In other words, yes if you use a +route.ts file then you are free not to use the Route type and no assignment type checking will be performed, true, but this won't work when directly assigning in +config.h.ts

True 👍. A solution would be to use pageContext: PageContextServer for Config['route'] (instead of setting it to RouteSync | RouteAsync). Alternatively, we create RouteWithServerRouting and set Config['route'] to RouteSync | RouteAsync | RouteWithServerRoutingSync | RouteWithServerRoutingAsync. Either way is fine with me.

I know it's very ugly for Server Routing users and only slightly prettier for Client Routing users. That's how much I think we should prioritize Client Routing over Server Routing... (I'd be up for deprecating Server Routing after all).

(if you agree, let's merge 🚀)

👍 I think you're right, the slight mismatch won't bother Server Routing users that much.

To answer your question: yes, there are slight mismatches between PageContextServer and PageContextClient, the most notable one is that everything is Partial because TypeScript cannot now the value of passToClient. (See vike/shared/types.ts.)

Actually... I'm just realizing that PageContextClient is only correct for Client Routing users... For Server Routing users it should actually be PageContextClientWithServerRouting. I've just checked and PageContextBuiltInClientWithServerRouting and PageContextBuiltInClientWithClientRouting (that's the core diff) are quite similar though: I think this mismatch is ok.

This is actually a good example for why I'm inclined to deprecate Server Routing: in my experience the maintaince costs it adds isn't worth it.

After thinking more about it: I suggest we keep the implementation as is while we tell Server Routing users to not use these new types introduced by this PR. This clearly marks Server Routing users as second-class citizen which fits the long-term plan of encouraging users to use vike-{react,vue,solid} while eventually deprecating Server Routing as soon it becomes too annoying to maintain.

The issues we're having with it in this PR makes me think it's a reason enough to deprecate it. I'm now inclined to think we should deprecate Server Routing before releasing v1.0.0.

(if you agree, let's merge )

👍 Let me know if you don't object with that last comment and I'll merge.

This sounds scary - aren't there valid use cases for not wanting client routing? I'll trust your guts on this

(whether or not we deprecate server routing, I think this PR is good enough to be merged)

While Server Routing is marginally faster on first page load (less client-side code to load), Client Routing is tremendously faster on subsequent navigation. I find this is a reason enough to recommend Client Routing over Server Routing.

Also there are use cases that only work with Client Routing (e.g. a music player that should still play music when navigating pages). That's the biggest reason why I'd always recommend using Client Routing. (You never know what use case you may need in the future.)

The idea of supporting Server Routing was neat but, in hindsight, I think it was a mistake.

All in all, I think it's worth it to deprecate it.

lourot · 2023-10-12T17:38:25Z

(all the rest looks good to me 👍)

brillout · 2023-10-12T17:45:48Z

(We can ignore Cloudflare's CI failing: we've reached Cloudflare's free tier daily rate liming.)

brillout · 2023-10-12T19:19:28Z

Merged 🎉

Thanks for the PR. Can't wait to release these really neat types.

lourot mentioned this pull request Sep 21, 2023

qual: improvements around getTitle vikejs/vike-react#20

Merged

lourot commented Sep 21, 2023

View reviewed changes

vite-plugin-ssr/shared/page-configs/Config.ts Outdated Show resolved Hide resolved

brillout force-pushed the main branch from 1f0d96d to 9dee181 Compare September 23, 2023 16:55

qual: make config hook types more precise

aaecd95

lourot force-pushed the quality/config-hook-types branch from bc08dbe to aaecd95 Compare September 27, 2023 14:05

lourot added 3 commits September 28, 2023 13:53

qual: fix type

7fbda5b

qual: fix CI failure

ed6e4d5

qual: use more precise types for onBeforeRender

22968d2

lourot commented Sep 28, 2023

View reviewed changes

vike/shared/page-configs/Config.ts Outdated Show resolved Hide resolved

qual: support server-and-client in onBeforeRender's type

8bde0ef

lourot commented Sep 29, 2023

View reviewed changes

vike/shared/page-configs/Config.ts Outdated Show resolved Hide resolved

qual: make config hook types more precise

3591539

lourot commented Sep 29, 2023

View reviewed changes

examples/react-full-v1/renderer/+onRenderHtml.tsx Outdated Show resolved Hide resolved

qual: fix types

9ddafd1

lourot commented Sep 29, 2023

View reviewed changes

examples/path-aliases-v1/renderer/onRenderHtml.tsx Outdated Show resolved Hide resolved

qual: use interface merging

fb693ef

lourot commented Sep 30, 2023

View reviewed changes

examples/path-aliases-v1/renderer/onRenderHtml.tsx Outdated Show resolved Hide resolved

lourot added 2 commits September 30, 2023 18:09

qual: improve onBeforeRender's and onRenderHtml's types

50ffcff

qual: improve onRenderClient's type

fd80d7e

experiment: stop supporting sync variant for void-returning hooks

5b4e15c

lourot commented Oct 12, 2023

View reviewed changes

lourot added 3 commits October 12, 2023 18:16

Revert "experiment: stop supporting sync variant for void-returning h…

35efab5

…ooks" This reverts commit 5b4e15c.

qual: harmonize hook types

fc31669

qual: remove redundant param types

25b9ce1

brillout added 4 commits October 12, 2023 18:49

suggestion: add link to docs instead of comment. That's how I've docu…

3b2971f

…mented Vike.PageContext on the boierlplates. But we can also add add a note if you want; as you wish.

suggestion: add @depreated to RouteAsync and provide Route type alias

91b3653

add argument to OnHydrationEnd{Async,Sync}

da76690

suggestion: assume user is using Client Routing (fairly safe assumpti…

9b7302b

…on in the future as we'll incrinsingly promote the vike-{react,vue,solid} packages). When the user uses Client Routing then the route functions are called on the server-side as well as on the client-side

lourot commented Oct 12, 2023

View reviewed changes

Merge branch 'main' into quality/config-hook-types

fb51afa

brillout merged commit b7465a5 into main Oct 12, 2023
30 of 32 checks passed

lourot deleted the quality/config-hook-types branch October 12, 2023 20:19

lourot mentioned this pull request Oct 13, 2023

docs: typescript hook documentation #1172

Merged

qual: make config hook types more precise #1137

qual: make config hook types more precise #1137

Conversation

lourot commented Sep 21, 2023

lourot commented Sep 21, 2023

lourot commented Sep 27, 2023

lourot commented Sep 27, 2023

brillout commented Sep 27, 2023

brillout commented Sep 27, 2023

brillout commented Sep 27, 2023

lourot commented Sep 27, 2023

brillout commented Sep 27, 2023

brillout commented Sep 28, 2023

lourot commented Sep 28, 2023

brillout commented Sep 28, 2023

brillout commented Oct 12, 2023

brillout commented Oct 12, 2023

lourot commented Oct 12, 2023

Choose a reason for hiding this comment

brillout commented Oct 12, 2023

brillout commented Oct 12, 2023

lourot commented Oct 12, 2023

lourot commented Oct 12, 2023

brillout commented Oct 12, 2023 • edited Loading

brillout commented Oct 12, 2023

brillout commented Oct 12, 2023

brillout commented Oct 12, 2023

lourot commented Oct 12, 2023

brillout commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lourot Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lourot commented Oct 12, 2023

brillout commented Oct 12, 2023

brillout commented Oct 12, 2023

brillout commented Oct 12, 2023 •

edited

Loading

lourot Oct 12, 2023 •

edited

Loading