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.

Sign up for GitHub

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

Jump to bottom

View Transition Animation Not Working for Async Components in MDX Files #11531

Open
1 task done
double-thinker opened this issue Jul 23, 2024 · 6 comments
Open
1 task done

View Transition Animation Not Working for Async Components in MDX Files #11531

double-thinker opened this issue Jul 23, 2024 · 6 comments
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: view transitions Related to the View Transitions feature (scope) pkg: mdx Issues pertaining to `@astrojs/mdx` integration requires refactor Bug, may take longer as fixing either requires refactors, breaking changes, or considering tradeoffs

Comments

@double-thinker
Copy link

Astro Info

Astro                    v4.12.2
Node                     v18.20.3
System                   Linux (x64)
Package Manager          unknown
Output                   static
Adapter                  none
Integrations             @astrojs/mdx

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

No response

Describe the Bug

Async components (with a module-level await) do not work with view transitions only if rendered within an MDX page. The same async component is correctly animated within a standard Astro page.

What's the expected result?

The animation should work regardless of whether it is used within MDX or not.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-p2qmkm?file=src%2Fpages%2Findex.astro

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 Jul 23, 2024
@martrapp
Copy link
Member

Hi @double-thinker, what a cool reproduction!

I have to admit that I have no idea what's going on at the moment. It might take some time to analyze and fix this.

Thanks for the offer to submit a PR. Since you did, do you happen to have any idea what's causing this behavior yet?

@martrapp martrapp removed the needs triage Issue needs to be triaged label Jul 24, 2024
@bluwy bluwy added - P3: minor bug An edge case that only affects very specific usage (priority) feat: view transitions Related to the View Transitions feature (scope) pkg: mdx Issues pertaining to `@astrojs/mdx` integration labels Jul 24, 2024
@martrapp
Copy link
Member

The HTML sources of the two iframes only differ in one item. The CSS for the view transition of the second paragraph is missing in the MDX case. The CSS gets generated for the Async.astro component and is added to the _metadata.extraHead of the SSRResult.

The special thing about the Async.astro component is that it yields to the MacrotaskQueue in the script part of the component.

While the generated CSS makes it from the _metadata to the <head> when the component is used on an Astro page, it gets lost for the MDX page.

Maybe there is a missing await or the <head> closed to early?
Fixing this is beyond my Astro skill & understanding. ;-) I will ask for help.

@double-thinker
Copy link
Author

double-thinker commented Jul 25, 2024 •
edited
Loading

Maybe it is related with this #7761 and withastro/roadmap#707:

Due to Astro's streaming, we only wait until the page's frontmatter has run before generating the element. Frontmatter of a component cannot add scripts or stylesheets because by the time it's run, the is already sent to the browser so that it can start loading assets.

This becomes an issue when a component uses the content collections API. The render() call adds styles and scripts relevant to the content entry to the element. Therefore, it must be used nowhere other than the routed page's frontmatter, as that would lead to missing assets:

@double-thinker
Copy link
Author

double-thinker commented Jul 25, 2024 •
edited
Loading

Ok, I'm not 100% sure, but it seems like this issue is related to the problem described in withastro/roadmap#707.

All transition:* props in an async component cannot add head CSS because it's already sent to the client. A workaround is to wrap the async element and move up the transition:* properties to the parent:

<div transition:animation="slide">
   <AsyncElement />
</div>

See the workaround here: https://stackblitz.com/edit/github-p2qmkm-bhkwwb?file=src%2Fcomponents%2FExample.astro

@bluwy bluwy added the requires refactor Bug, may take longer as fixing either requires refactors, breaking changes, or considering tradeoffs label Jul 25, 2024
@bluwy
Copy link
Member

bluwy commented Jul 25, 2024

This one is a bit hard to fix. The gist of the problem goes like this:

In .astro rendering, we have a prepass rendering phase where we do a quick init and crawl for Astro components recursively and we can detect which Astro components need head rendering. After that prepass, we can explicit render the heads of those components ahead, and by that time, result._metadata.extraHead will have the VT styles appended, good to go until the actual head render call later.

In .mdx (or any non-Astro page), they don't have a prepass feature. The pages are rendered as is, which means any promises will not be awaited if it later contributes any head. If you try to move that higher up to emulate a prepass, it's not going to work because it will do the actual head render call earlier. A prepass phase would not do any head render calls.

Ultimately, it's hard to fix this. We could try to create a prepass for .mdx but I'm not sure how well that scales for non-Astro pages. We could try an easy patch where we append later-added extraHead (like this), but it's an ugly hack and will likely cause more bugs in the future.

Another example why streaming is a PITA 🫠 I'm not sure how to do a nice fix for this at the moment, a workaround on the user-side may be needed.

Ok, I'm not 100% sure, but it seems like this issue is related to the problem described in withastro/roadmap#707.

It's not directly related I think as that proposal is for fixing renders in Astro files.

@double-thinker
Copy link
Author

If you try to move that higher up to emulate a prepass, it's not going to work because it will do the actual head render call earlier. A prepass phase would not do any head render calls.

However, the workaround that moves them up is actually working. It seems like an MDX with sync components has no problem putting VT's CSS in extraHead.

Ok, I'm not 100% sure, but it seems like this issue is related to the problem described in withastro/roadmap#707.
It's not directly related I think as that proposal is for fixing renders in Astro files.

From my understanding, the solution proposed in #707 could help here. If any non-Astro component has export const addsToHead, then the server should wait until they are rendered to send the head. IMO this solution is understandable for users, and it could be generalized to almost any framework. The other solution also helps: Astro.waitFor(<Content />) will explicitly ask to wait until the render finishes before sending the head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: view transitions Related to the View Transitions feature (scope) pkg: mdx Issues pertaining to `@astrojs/mdx` integration requires refactor Bug, may take longer as fixing either requires refactors, breaking changes, or considering tradeoffs
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@double-thinker @bluwy @martrapp