brisa-build / brisa

The Web Platform Framework.
https://brisa.build
MIT License
449 stars 12 forks source link

SPA navigate() renders wrong page #558

Closed AlbertSabate closed 1 month ago

AlbertSabate commented 1 month ago

Describe the bug When you are using navigate() to navigate from '/lala/lolo' to navigate('/lala'). The page that will be rendered is /lala/lolo instead of /lala.

You need to navigate using SPA from another page. Example:

/lala/lolo navigate('/lala/lele'); -> works /lala/lele navigate('/lala'); -> will render /lala/lele instead of /lala

Expected behavior The page that should be rendered is /lala instead of /lala/lele.

aralroca commented 1 month ago

It only happens with navigate function or also with anchor <a /> tagName?

AlbertSabate commented 1 month ago

I can confirm this is not happening when using /lala. It navigates properly.

aralroca commented 1 month ago

@AlbertSabate, can you tree your files? and where are using navigate, in a server action (server) or a WC browser event (client)?

AlbertSabate commented 1 month ago
Screenshot 2024-10-16 at 6 01 01 PM

All server components.

Happens when I navigate from /enquiry/client-details.tsx to /enquiry/trip-details to /enquiry, this last does not work.

Implementation is as follows:

// client-details
function backBtn() {
  navigate('/enquiry/trip-details');
}

// trip-details
function backBtn() {
  navigate('/enquiry');
}
aralroca commented 1 month ago

Okay, I think I have detected the root of the problem.

I think the problem comes from the client RPC, which is in charge of sending the POST with the actionId, after browsing the old ones are unregistered and the new actions are registered.

https://github.com/brisa-build/brisa/blob/0d5c01fd07641d7601c0a94bf6233516ae07738b/packages/brisa/src/utils/rpc/rpc.ts#L46

Adding a console.log(actionId) looks that is not upgraded after navigation.

Once they are registered, they are only unregistered when the element is removed. That's wrong.

https://github.com/brisa-build/brisa/blob/0d5c01fd07641d7601c0a94bf6233516ae07738b/packages/brisa/src/utils/rpc/register-actions/index.ts#L19-L37

Also that comment is wrong:

// It is registered once, when diffing the navigation, if the element 
// is the same, the action attribute (data-action) is not added and 
// therefore it is not added again, only the new elements that have 
// the data-action are registered.

Even if it is already registered, the element may have another server action on another page, and this is what is not being done right.

aralroca commented 1 month ago

This is also incorrect:

https://github.com/aralroca/diff-dom-streaming/blob/cac8ca18ea4312fe91b7155f95c65fb06b5c5366/src/index.ts#L110

aralroca commented 1 month ago

@AlbertSabate Meanwhile this is not fixed, as a workaround you can add a different key, example based on the route name:

import { navigate, type RequestContext } from "brisa";

export default function Nav({}, { route}: RequestContext) {
  return (
    <>
      <h1>Lala / Lele</h1>
      <button key={route.name} onClick={() => navigate('/lala')}>Go to /lala</button>
    </>
  )
}

This way is not re-using the same element and avoiding this issue.

aralroca commented 1 month ago

The solution we can keep all the current RPC code, in fact if it is ok. It is not necessary to get the value in runtime of each event, because there would still be a problem, that only that the same component in a page has an onClick, and in the other page an onDblClick, when not deleting the old events it would enter in conflict again.

Then the solution would come in using the data-cid in the diffing as if it was a key, that is an attribute that Brisa puts when there are server actions and it needs it of reference, that this if it is different in each render. This way, I would know that they are different elements although visually they are the same, and I would clean all the old actions and mount the new ones (that the current RPC code already does it).

AlbertSabate commented 1 month ago

Confirming that with key="blahblah" works fine.

aralroca commented 1 month ago

@AlbertSabate Solved in 0.1.2-canary.3

AlbertSabate commented 1 month ago

Confirm working on 0.1.2-canary.3

aralroca commented 1 month ago

I re-open because is not fixed in the best way