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

Partial rendering of elements is out of order #1950

Closed
salvaft opened this issue Oct 17, 2023 · 11 comments · Fixed by #1958
Closed

Partial rendering of elements is out of order #1950

salvaft opened this issue Oct 17, 2023 · 11 comments · Fixed by #1958
Labels
bug Something isn't working.

Comments

@salvaft
Copy link

salvaft commented Oct 17, 2023

I have this page component. Based on the query params the h2 should be displayed or not. I am using a form to update the query params. If i do a search and I hit the back button or click a link to come back, the h2 heading is rendered AFTER.

Here is the video:
simplescreenrecorder-2023-10-17_14.29.13.webm

I added f-client-nav to the body tag. Created a _layout.tsx and using the page function component as a whole as a partial.

Here is the link to code sand box: https://codesandbox.io/p/sandbox/compassionate-yalow-f3r7fr and this is the running example: https://f3r7fr-8000.csb.app/

 export default async function Home(
  req: Request,
  ctx: RouteContext<any, State>,
) {
  const url = new URL(req.url);
  const search = url.searchParams.get("search") || "";

  let products: Product[] | undefined;
  if (search !== "") {
    products = await ctx.state.store.getProductsByName(search);
  } else {
    products = await ctx.state.store.getProducts({ limit: 5, offset: 0 });
  }

  return (
    <Partial name="main">
      {search === "" && (
        <h2 class="my-4 text-center text-2xl font-bold leading-9 tracking-tight text-gray-200">
          Featured products
        </h2>
      )}
      <div class="m-4 p-4 border-blue-300 border-2 rounded-xl ">
        {products && <ProductList products={products}></ProductList>}
      </div>
    </Partial>
  );
}
@salvaft salvaft changed the title Partial rendering out of order Partial rendering of elements is out of order Oct 17, 2023
@marvinhagemeister
Copy link
Collaborator

marvinhagemeister commented Oct 18, 2023

Took a closer look and the issue stems from there being two <Partial name="main"> components on the same page. The name of the Partial is supposed to be unique, but because there are two components with the same name Fresh gets confused as to which component the content should be inserted to. That makes it look like the heading moved.

We should throw a warning or an error when a partial with the same name is detected.

@salvaft
Copy link
Author

salvaft commented Oct 18, 2023

I think then I misunderstood the API and the <Partial> in the layout is not neccesary. Either i wrap the content in the layout or the whole return of the page.

However if I remove it still does not work. The form handler is updating the params, it is the same page.

I think I am not getting it.

@marvinhagemeister
Copy link
Collaborator

Your app renders two Partials with the same name. Inside _app.tsx there is <Partial name="main"> and inside index.tsx there is the same partial name again. This leads to this structure of partials being rendered:

<Partial name="foo">
  <Partial name="foo">...</Partial>
</Partial>

In your case you don't need the partial in index.tsx By default the content of that file is placed to where <Component /> is called in _app.tsx. Replace that with a Fragment and your app works as expected.

  export default async function Home(
    // ...snip
   
    return (
-     <Partial name="main">
+     <>
        {search === "" && (
          <h2 class="my-4 text-center text-2xl font-bold leading-9 tracking-tight text-gray-200">
            Featured products
          </h2>
        )}
        <div class="m-4 p-4 border-blue-300 border-2 rounded-xl ">
          {products && <ProductList products={products}></ProductList>}
        </div>
-     </Partial>
+     </>
    );
  }

@salvaft
Copy link
Author

salvaft commented Oct 18, 2023

I guess you meant _layout.tsx? There is no <Partial name="main"> in _app.tsx. What I tried to say in the previous comment is that even if I remove it from _layout.tsx and leave it in the page, or leave it in the page and remove it from layout it is still not working (already removed in code sandbox link)

Thanks in advance for your time

@marvinhagemeister
Copy link
Collaborator

oh sorry yeah you're right, meant _layout.tsx. Putting it there is fine, but putting another one into a route component on top of that is not.

@marvinhagemeister
Copy link
Collaborator

marvinhagemeister commented Oct 18, 2023

@SFToro Apply this patch to your project to resolve the issue.

diff --git a/islands/SearchForm.tsx b/islands/SearchForm.tsx
index 4338824..01ce92f 100644
--- a/islands/SearchForm.tsx
+++ b/islands/SearchForm.tsx
@@ -22,7 +22,7 @@ export default function SearchForm() {
           class="space-y-6"
           id="search-form"
           action="/products"
-          method="POST"
+          method="GET"
           onSubmit={handleSubmit}
         >
           <div>
diff --git a/routes/index.tsx b/routes/index.tsx
index 90fe802..bf7867c 100644
--- a/routes/index.tsx
+++ b/routes/index.tsx
@@ -19,7 +19,7 @@ export default async function Home(
   }
 
   return (
-    <Partial name="main">
+    <>
       {search === "" && (
         <h2 class="my-4 text-center text-2xl font-bold leading-9 tracking-tight text-gray-200">
           Featured products
@@ -28,6 +28,6 @@ export default async function Home(
       <div class="m-4 p-4 border-blue-300 border-2 rounded-xl ">
         {products && <ProductList products={products}></ProductList>}
       </div>
-    </Partial>
+    </>
   );
 }

@salvaft
Copy link
Author

salvaft commented Oct 18, 2023

Hi again,
Thanks for the help. The post method was there because i was playing with partial form submission (the version running in code sandbox will always fetch with get).

What I tried to say in the previous comment is that even if I remove it from _layout.tsx and leave it in the page, or leave it in the page and remove it from layout it is still not working (already removed in code sandbox link)

Anyway what I am trying to say is that i applied such patch in the layout and i am still getting the same behaviour

<>
      {search === "" && (
        <h2 class="my-4 text-center text-2xl font-bold leading-9 tracking-tight text-gray-200">
          Featured products
        </h2>
      )}
      <div class="m-4 p-4 border-blue-300 border-2 rounded-xl ">
        {products && <ProductList products={products}></ProductList>}
      </div>
   </>
<>
   <header class="px-4 py-8 mx-auto bg-[#86efac]">
     <div class="max-w-screen-md mx-auto flex flex-col items-center justify-center">
       <a href="/">
         <img
           class="mt-4"
           src="/logo.svg"
           width="64"
           height="64"
           alt="the Fresh logo: a sliced lemon dripping with juice"
         />
       </a>
       <SearchForm></SearchForm>
     </div>
   </header>
   <main>
     <Partial name="main">
       <Component />
     </Partial>
   </main>
 </>

still same behaviour

@marvinhagemeister
Copy link
Collaborator

marvinhagemeister commented Oct 18, 2023

@SFToro Can you record a quick video of that? nevermind I can reproduce it, sorry for the back and forth.

@marvinhagemeister marvinhagemeister added the bug Something isn't working. label Oct 18, 2023
@salvaft
Copy link
Author

salvaft commented Oct 18, 2023

simplescreenrecorder-2023-10-18_20.21.00.webm
The back navigation is an anchor tag on the lemon logo. But rendering out of order also happens hitting the back button.

@marvinhagemeister
Copy link
Collaborator

Just leaving some debugging notes for myself here:

  1. We use a virtual container DOM node to render in-between DOM nodes instead of rendering into a container
  2. children props are all updated correctly when partial updates are applied
  3. We run into the first branch here https://github.com/preactjs/preact/blob/19de3d9688441a227af3a9edb521905e4b3d8195/src/diff/children.js#L305-L306 , because dom.parentNode is different with a virtual DOM container node which leads to null being passed to insertBefore. That leads to the element being moved to the end

@marvinhagemeister
Copy link
Collaborator

This is resolved by upgrading to Preact 10.19.2. This can be done manually in deno.json. The next version of Fresh ships with a newer Preact version out of the box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants