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

(router) improve static pages #885

Open
dai-shi opened this issue Sep 16, 2024 · 12 comments
Open

(router) improve static pages #885

dai-shi opened this issue Sep 16, 2024 · 12 comments
Assignees

Comments

@dai-shi
Copy link
Owner

dai-shi commented Sep 16, 2024

Background

The two rendering types "static" and "dynamic" are very complicated because there are two aspects RSC and SSR.

This isStatic is for RSC:

isStatic?: boolean | undefined;

This `isStatic is for SSR:

isStatic?: boolean | undefined;

Currently, isStatic for SSR will be true if all of isStatic for RSC are true.

Expectation

If we mark a "page" or a "layout" to be "static", our expectation is that the rendering only happens just once. (For PRD, only at the build time, and for DEV only the first time and again after hot reload.)

Actual problem

If we mix "static" and "dynamic" for RSC, the page for SSR will be "dynamic" which is expected, but then RSC's "static" isn't truly static.

In the 01_template, modify _layout.tsx to be "dynamic", while keeping "static" in pages.tsx. Now, HomePage in page.tsx renders more than expected. In addition to the build time, it renders on production. It only behaves like "static" on client routing thanks to shouldSkip mechanism.

Solution

Challenges

  • In DEV, it would be nice if the behavior is identical to PRD, but we need to delete the RSC text cache (hm, where do we store it?) after hot reload.
  • shouldSkip should be revisited as it's too complicated. (It's a separate issue and more important than this one.)
  • We may have more types than "static" and "dynamic" in the future, but we can reconsider our design at that time. (Trial-and-error is welcome.)
@dai-shi
Copy link
Owner Author

dai-shi commented Sep 16, 2024

@tylerlaws0n here's the first write-up.

@tylersayshi
Copy link
Contributor

@dai-shi Are you suggesting that the static page with a dynamic layout should always become implicitly dynamic to account for the dynamic layout's ability to change?

I think it might help to talk through a scenario where you'd want this mixing of build modes between the layout and the page itself.

Without coming into this problem yet myself, I would think that these two things should simply not be mixed. I feel like if you dynamically update the static page, that is unexpected behavior. And if you statically render a dynamic layout, that is also unexpected.

I am probably missing something though since I don't have a full picture on what the idea is, so if you could correct me where I am missing info that would be helpful. Thanks!

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 24, 2024

Are you suggesting that the static page with a dynamic layout should always become implicitly dynamic to account for the dynamic layout's ability to change?

I'm not suggesting it in this issue, but it's how it works as of now.

I would think that these two things should simply not be mixed.

What about static layout and dynamic page? Feels like a common case.
We can cache layout component without refetching when we reload.
(It's currently handled with shouldSkip.)

I am probably missing something though since I don't have a full picture on what the idea is, so if you could correct me where I am missing info that would be helpful.

No problem! It's understandable. (Actually, by going through this task with you, I'm expecting to discuss some other (not implemented yet) features with you in the future.

@tylersayshi
Copy link
Contributor

What about static layout and dynamic page

This use case makes a lot of sense and definitely should be supported.

With my last message, I was focused on "static page + dynamic layout" which still feels like an error case to me.

I'm expecting to discuss some other (not implemented yet)

Sounds good, looking forward to that!

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 25, 2024

static layout + dynamic page is more common, but dynamic layout+ static page is also possible. I don't think we need to prohibit it if we can support it easily.

@tylersayshi
Copy link
Contributor

@dai-shi do any of our examples currently show the dynamic layout + static page pattern currently? If no, I can start this issue by just spinning up an example for us to reference and test.

@dai-shi
Copy link
Owner Author

dai-shi commented Sep 26, 2024

I'm not sure, but maybe not. Yeah, sounds good.

tylersayshi added a commit to tylersayshi/waku that referenced this issue Sep 30, 2024
you can see the time change as navigating the site which shows
that the layout is indeed dynamic

The rest of the home page stays unchanged and can be cached

Example for dai-shi#885
@tylersayshi
Copy link
Contributor

Added the example for now... I'll follow up with improvements separately if that's ok. This helps to test and see the current behavior though.

@tylersayshi
Copy link
Contributor

tylersayshi commented Oct 3, 2024

If we mix "static" and "dynamic" for RSC, the page for SSR will be "dynamic" which is expected, but then RSC's "static" isn't truly static.

Does this have to be true?

If the desired outcome is to have a true static RSC (rendered only at build time) then couldn't waku save the RSC payload of that static component.

edit: I see now that you suggested this in the solution section: As we now have buildData #860, we can store RSC text for "static" RSC, even if the SSR is dynamic.

@tylersayshi
Copy link
Contributor

I understand the problem and proposed fix now though. I can take this on alongside work on the query params.

dai-shi pushed a commit that referenced this issue Oct 3, 2024
you can see the time change as navigating the site which shows that the
layout is indeed dynamic

The rest of the home page stays unchanged and can be cached

Example for #885

---------

Co-authored-by: Tyler <26290074+thegitduck@users.noreply.github.com>
@tylersayshi
Copy link
Contributor

@dai-shi Correct me here if I am misunderstanding, but this code: https://github.com/dai-shi/waku/blob/main/packages/waku/src/router/define-router.ts#L242-L252 and similar snippets in both defineRouter and createPages are currently assuming 1:1 relationship from path to component id.

So the improvement for this issue is to:

  1. detect all RSC's for a given path
  2. generate a new component id for each RSC
  3. when a path is requested check for all RSC ids that that path has, then read from cache to find cache hits
  4. finally weave the results and return

Questions:

  1. should the full path also still be cached, or not anymore if more refined caching exists? (for SSR + dynamic)
  2. How would you approach finding all RSC's given a path?

@dai-shi
Copy link
Owner Author

dai-shi commented Oct 5, 2024

So the improvement for this issue is to:

1&2 are the current behavior, right?

should the full path also still be cached

The full path is cached, already in a file. so we don't need it in buildData. (Or, maybe we can cache them regardless.)

How would you approach finding all RSC's given a path?

It's getComponentIds as of now.

HOWEVER:

currently assuming 1:1 relationship from path to component id.

I would like to change this 1:1 relationship. It's one of the reasons behind unstable_ prefix.
Your question gives me a hint, actually. Let's work on a new defineRouter, and if it seems to go well, we can revisit this issue later (or at the same time.)

dai-shi added a commit that referenced this issue Oct 10, 2024
We would like to redesign `defineRouter`. This is why we have
`unstable_` prefix.

#885 (comment)

> similar snippets in both `defineRouter` and `createPages` are
currently assuming 1:1 relationship from path to component id.

That's the exactly one of the reasons. It shouldn't be 1:1. It can be
anything, and it can cache each. This will change waku/router/client too
including "shouldSkipList".

Starting with a rough idea.
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