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

Partytown breaks VT fallback #8862

Closed
1 task
florian-lefebvre opened this issue Oct 18, 2023 · 27 comments · Fixed by #9419
Closed
1 task

Partytown breaks VT fallback #8862

florian-lefebvre opened this issue Oct 18, 2023 · 27 comments · Fixed by #9419
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) ecosystem: external External library doesn't work feat: view transitions Related to the View Transitions feature (scope)

Comments

@florian-lefebvre
Copy link
Member

Astro Info

Astro                    v3.3.2
Node                     v18.14.1
System                   Windows (x64)
Package Manager          yarn
Output                   static
Adapter                  none
Integrations             @astrojs/mdx
                         @astrojs/sitemap
                         @astrojs/partytown

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

When creating a partytown script and navigating to a new page in an unsupported browser (like firefox), VT breaks: url is not updated, event listeners are not triggered etc but page swaps. See video.

image

Astro.Blog.Firefox.Developer.Edition.2023-10-18.15-48-05.mp4

What's the expected result?

Shouldn't break

Link to Minimal Reproducible Example

https://github.com/florian-lefebvre/astro-vt-fallback-bug

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Oct 18, 2023
@florian-lefebvre florian-lefebvre changed the title Partytown breaks VT fllback Partytown breaks VT fallback Oct 18, 2023
@martrapp martrapp assigned martrapp and unassigned martrapp Oct 18, 2023
@martrapp martrapp added help wanted Please help with this issue! and removed needs triage Issue needs to be triaged labels Oct 18, 2023
@martrapp
Copy link
Member

Help wanted! Obviously the minimal example does not work for @florian-lefebvre, but I was not able to reproduce the error with Firefox. Who can help to identify the source of this behavior?

@lilnasy
Copy link
Contributor

lilnasy commented Oct 18, 2023

I can reproduce the issue. I could take a closer look later this week.

video.mp4

@lilnasy
Copy link
Contributor

lilnasy commented Oct 18, 2023

@alan-barzilay
Copy link

I also got the same issue, firefox + partytown + VT

alan-barzilay added a commit to alan-barzilay/sidon that referenced this issue Oct 23, 2023
partytown + firefox + view transitions is broken:
withastro/astro#8862

besides, it seems like my scripts cant be used with partytown

This reverts commit 8494403.
@lilnasy
Copy link
Contributor

lilnasy commented Oct 23, 2023

This one might be hard to debug. The Firefox-specific error does not provide a stacktrace. I'm pretty sure it has to do with Partytown's usage of low-level APIs (Atomics, SharedArrayBuffer) that exposes a memory-management bug in Firefox, but there is no straightforward way to confirm.

@lilnasy lilnasy added - P4: important Violate documented behavior or significantly impacts performance (priority) ecosystem: external External library doesn't work feat: view transitions Related to the View Transitions feature (scope) labels Oct 23, 2023
@lilnasy
Copy link
Contributor

lilnasy commented Oct 23, 2023

Workaround: turn off minification.

import { defineConfig } from 'astro/config';
import mdx from '@astrojs/mdx';
import sitemap from '@astrojs/sitemap';

import partytown from "@astrojs/partytown";

// https://astro.build/config
export default defineConfig({
  site: 'https://example.com',
  integrations: [mdx(), sitemap(), partytown()],
+ vite: {
+   build: {
+     minify: false
+    }
+  }
});

@lilnasy lilnasy added - P2: has workaround Bug, but has workaround (priority) and removed - P4: important Violate documented behavior or significantly impacts performance (priority) labels Oct 23, 2023
@florian-lefebvre
Copy link
Member Author

Do you think it's worth opening an issue usptream or is it really caused by Astro?

@florian-lefebvre
Copy link
Member Author

I deployed a vercel preview with your suggested workaround because it didn't work locally, and it didn't work either. You can check it out at https://legismusicastro-lfkizaw6x-legis-music.vercel.app

@lilnasy lilnasy added - P4: important Violate documented behavior or significantly impacts performance (priority) and removed - P2: has workaround Bug, but has workaround (priority) labels Oct 24, 2023
@lilnasy
Copy link
Contributor

lilnasy commented Oct 24, 2023

Do you think it's worth opening an issue usptream or is it really caused by Astro?

I'm not sure yet. Since, view transitions are necessary for it to break on Firefox, there might be a fix possible on Astro's side. Just as long as it's not too drastic.

@lilnasy
Copy link
Contributor

lilnasy commented Oct 24, 2023

Using a build of partytown with minification turned off pointed me to a single line.

image

Partytown patches the History API, and its code runs whenever the History API is used. It seems like an issue for Firefox to fix, although Partytown may be able to remove this hacky code. Astro's usage of history API is necessary and proper.

@lilnasy
Copy link
Contributor

lilnasy commented Nov 2, 2023

Reducing priority only because it is a signal astro maintainers use to pick up issues. This is not something for them to fix.

@emish89
Copy link
Contributor

emish89 commented Nov 30, 2023

I created the bug here, maybe this can help:
BuilderIO/partytown#505

@lilnasy
Copy link
Contributor

lilnasy commented Nov 30, 2023

@matthewp
Copy link
Contributor

Might be able to prevent it by stopping Partytown from highjacking pushState, going to give that a try.

@matthewp
Copy link
Contributor

Looks like that will work.

@emish89
Copy link
Contributor

emish89 commented Dec 12, 2023

@matthewp can you post a code snippet?
in this way we are losing GA events, correct?

@matthewp
Copy link
Contributor

@emish89 I can create a preview release for you to try. You might lose events, I'm not sure. Hopefully not.

@matthewp
Copy link
Contributor

@emish89 could you try out astro@experimental--vt-partytown and let me know?

@emish89
Copy link
Contributor

emish89 commented Dec 12, 2023

ok tomorrow I'm going to try.
I also created a PR in partytown that should solve :/
BuilderIO/partytown#511

@matthewp
Copy link
Contributor

@emish89 have you confirmed that removing the setTimeout does fix it?

@lilnasy
Copy link
Contributor

lilnasy commented Dec 12, 2023

It was not sufficient to prevent the failure when I tried it. It has been a while though, it's possible I missed something.

@emish89
Copy link
Contributor

emish89 commented Dec 12, 2023

in my local worked, but for sure I'm not an expert of the package. If they put it, is because there was some reason...

@lilnasy do you remember what can I test to be sure if is working or not?

@lilnasy
Copy link
Contributor

lilnasy commented Dec 12, 2023

If firefox does not error anymore then disregard what I said. The only tests to worry about would be the ones in partytown repo.

@matthewp
Copy link
Contributor

@emish89 did you get a chance to try out astro@experimental--vt-partytown?

@emish89
Copy link
Contributor

emish89 commented Dec 13, 2023

I would say that is working, for sure is not breaking, but I'm not 100% sure if is sending data to GA.
I cannot install extensions to check that on firefox

@matthewp
Copy link
Contributor

Ok, I'm going to suggest that we go with this then. The most important thing is that page navigation is not broken. If Partytown is causing it to be so, then the priority should be for us to avoid that.

@emish89
Copy link
Contributor

emish89 commented Dec 14, 2023

Ok, I'm going to suggest that we go with this then. The most important thing is that page navigation is not broken. If Partytown is causing it to be so, then the priority should be for us to avoid that.

totally understandable and I agree. I confirm that in my case is now not breaking with this version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) ecosystem: external External library doesn't work feat: view transitions Related to the View Transitions feature (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants