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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[馃悶] SPA resume (probably) needs to be fixed for upcoming v2 changes #5983

Closed
jordanw66 opened this issue Mar 9, 2024 · 5 comments
Closed
Labels
COMP: qwik-city P3: important If Bug - it makes Qwik unstable but doesn't affect the majority of users TYPE: bug Something isn't working VERSION: upcoming major

Comments

@jordanw66
Copy link
Contributor

jordanw66 commented Mar 9, 2024

Which component is affected?

Qwik City (routing)

Describe the bug

Explanation

SPA resume (popstate after refresh/reload) needed to exist outside of Qwik in order to keep it simple and not pull in anything that is not already being used on a page.

For this reason, when a popstate occurs on a page where SPA context was lost, it only directs the page back into the useNavigate pipeline when an already-existing useNavigate context exists on the page, and falls back to location.href refreshing.

This functionality solves all cases:

  • No SPA-related code loaded on MPA-only pages.
  • Pages with useNavigate ctx will be restored into the SPA pipeline.
  • Edgecase of "leaf node" which is arrived at via Link/useNavigate, but does not contain either, and a refresh occurs. It isn't possible to differentiate this page from a purely MPA during SSR, no useNavigate ctx can be included.

Problem

The simple and quick way of hooking back into the useNavigate ctx was to:

  • Check the page for a Link, specifically an a element with the attribute q:key="AD_1".
  • This elm then gets duplicated in a way that is invisible, and a new attribute q:nbs (Nav BootStrap) would be attached.
  • This attr tells the Link component to ignore the href attr and instead call: nav(location.href, { type: 'popstate' })
  • The modified Link elm gets click() and is pushed through the loader normally, resuming the app, everything just works.

This was simple even though it only works if the official Link component is present, and I didn't understand internals enough to decipher and jiu-jitsu a hand-crafted way to directly detect and hook any case of useNavigate ctx on the page from the qwik/json in the time available. (that would've needed adapting to v2 as well)

With the upcoming Qwik v2 changes as they appear in this blog post: https://www.builder.io/blog/qwik-2-coming-soon. It appears the current method of detecting a Link described above might not work anymore. Specifically because it relies on a q:key attribute on the a element. Can someone confirm this?

Solution(s)

If this does turn out to be an issue, some possible solutions:

  • Simplest: add new attr to the Link component's a element with a magic string and use that to select a Link on the page.
  • Harder/Ideal: Jiu-jitsu a way to detect any case of useNavigate, even with just nav() or custom Links, and somehow hook into this directly. This would remove dependence on the official Link component.

Unfortunately I do not currently have the time it would require to setup a dev environment on v2 and investigate and adjust this behavior. Hopefully the few minutes it took to explain this issue in detail is helpful to someone in solving this.

For reference, here are the current parts of the code that are described above:

https://github.com/BuilderIO/qwik/blob/157f7b01df22448638f7cc55546a051aa4eea5f0/packages/qwik-city/runtime/src/spa-init.ts#L74-L78

https://github.com/BuilderIO/qwik/blob/157f7b01df22448638f7cc55546a051aa4eea5f0/packages/qwik-city/runtime/src/link-component.tsx#L70-L72

Reproduction

N/A

Steps to reproduce

No response

System Info

N/A

Additional Information

No response

@jordanw66 jordanw66 added STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working labels Mar 9, 2024
@wmertens wmertens added COMP: qwik-city P3: important If Bug - it makes Qwik unstable but doesn't affect the majority of users VERSION: upcoming major and removed STATUS-1: needs triage New issue which needs to be triaged labels Mar 9, 2024
@wmertens
Copy link
Member

wmertens commented Mar 9, 2024

Thanks Jordan! Question: how about always including the spa context, would that also solve it? Is it that huge?

@jordanw66
Copy link
Contributor Author

jordanw66 commented Mar 9, 2024

Thanks Jordan! Question: how about always including the spa context, would that also solve it? Is it that huge?

I don't think this is possible, the useNavigate context requires the full Qwik City meta framework to also be present. Qwik's feature of choosing between SPA or MPA dynamically without having to make a permanent design decision constrains this because MPA doesn't want to load anything to do with SPA.

When I first designed the SPA resume and scroll restore it was clear to meet the demands for all cases, it had to exist kind of outside Qwik & Qwik City such that it will hook in to useNavigate if it's already included on a page to re-bootstrap but not actually trigger any dependency if it's not. This also solves edge-cases as explained above.

With that said, this can be fixed very easily by simply including a new attribute with a magic string on the Link component and selecting on that. This wouldn't require any other changes I think. But perhaps maybe someone can find a better way to expand functionality to work for all cases of useNavigate like nav() or custom Link components instead of relying only on the official Link.

@wmertens
Copy link
Member

wmertens commented Mar 9, 2024

Hmm I'm afraid I don't understand. Apart from the contexts' data that Link uses, what needs to be shipped to the client to enable SPA?

@jordanw66
Copy link
Contributor Author

jordanw66 commented Mar 9, 2024

Hmm I'm afraid I don't understand. Apart from the contexts' data that Link uses, what needs to be shipped to the client to enable SPA?

The useNavigate context needs to be shipped for SPA, this is big as it requires all of Qwik City, that's why SPA only resumes if it was already included on the page because the dev used Link or nav().

If this isn't present then it falls back to simply reloading the page with location.href when a popstate occurs. This will usually only occur on a "leaf node", a page arrived at from a navigate but which doesn't contain any Link or nav itself. (anti-pattern but needs to work anyway)

To make it more clear, this resume is included on every single Qwik City page even MPA. Except it's just a tiny couple-line shim, the actual script is gated behind a history.state check, this checks if the page was ever SPA, if it was then a much larger (pre-cached during previous SPA) script is manually loaded (bypasses the Qwik loader), this big one actually performs this resume functionality. But only if the context is already present on the page, because that pulls in many kB of Qwik City. It's like a multi-stage boot process, because we want to minimize footprint to near 0 when SPA is not used.

When a user refreshes a page that was arrived at through SPA, or re-opens browser, etc. the navigate context is lost. This whole contraption is designed to track and restore scroll positions no matter what the user does or how badly things get messed up and it has to do it without impacting MPA; it always just works and it's nearly free.

Hope that explains everything. And like I said above, there's a couple line solution to fix this problem and maintain current limitation (dependence on Link), I updated the issue to explain.

@PatrickJS
Copy link
Member

partially fixed #6022

@PatrickJS PatrickJS closed this as not planned Won't fix, can't repro, duplicate, stale May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP: qwik-city P3: important If Bug - it makes Qwik unstable but doesn't affect the majority of users TYPE: bug Something isn't working VERSION: upcoming major
Projects
None yet
Development

No branches or pull requests

3 participants