-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: fix up PrefetchServiceWorker with multiple Qwik containers #6468
Conversation
👷 Deploy request for qwik-insights pending review.Visit the deploys page to approve it
|
@builder.io/qwik (
|
LGTM, why is this draft @gioboa ? |
Because we are testing the fix in our POC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work 🙌
@@ -23,9 +23,21 @@ export function prefetchUrlsEventScript(prefetchResources: PrefetchResource[]) { | |||
const data: QPrefetchData = { | |||
bundles: flattenPrefetchResources(prefetchResources).map((u) => u.split('/').pop()!), | |||
}; | |||
return `document.dispatchEvent(new CustomEvent("qprefetch",{detail:${JSON.stringify(data)}}))`; | |||
return `(${PREFETCH_BUNDLES_CODE})( | |||
document.currentScript.closest('[q\\\\:container]'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need this? [q\\:container]
is not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to run this in the browser to use the right one. I used the same logic we have here
Overview
What is it?
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Use cases and why
Checklist: