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

MDX Layout css applied after component css on build #11040

Closed
1 task
claytercek opened this issue May 14, 2024 · 7 comments · Fixed by #11818
Closed
1 task

MDX Layout css applied after component css on build #11040

claytercek opened this issue May 14, 2024 · 7 comments · Fixed by #11818
Assignees
@bluwy
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) pkg: mdx Issues pertaining to `@astrojs/mdx` integration

Comments

@claytercek
Copy link

Astro Info

Astro                    v4.7.0
Node                     v20.10.0
System                   macOS (x64)
Package Manager          npm
Output                   static
Adapter                  none
Integrations             @astrojs/mdx
                         @astrojs/sitemap
                         @astrojs/react

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

No response

Describe the Bug

When using the mdx integration, if you specify a layout in the frontmatter, the CSS from that layout will be included after any imported component css when built.

In the linked stackoverflow, pages/index.mdx imports a component (Demo.astro) which has some component styles, and has a layout (RootLayout.astro) which includes some global styles that specify the layer order with the @layer directive.

Here's the relevant css:

/* global styles in RootLayout.astro  */

@layer reset, component;

@layer reset {
    * {
        background: white;
        color: black;
    }
}

/* styles in Demo.astro */

@layer component {
    div {
        background: red;
        color: blue;
    }
}

The compiled css ends up looking like this (whitespace added for readability):

@layer component {
    div[data-astro-cid-tb5vpudz] {
        background: red;
        color: #00f
    }
}

@layer reset, component;
@layer reset{
    * {
        background: #fff;
        color: #000
    }
}

Because the component layer is declared before the reset layer, the reset layer is taking precedence.

What's the expected result?

I'd expect the styles from the layout to be included before the styles from any imported components:

@layer reset, component;
@layer reset{
    * {
        background: #fff;
        color: #000
    }
}

@layer component {
    div[data-astro-cid-tb5vpudz] {
        background: red;
        color: #00f
    }
}

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-ceeysf?file=src%2Fcomponents%2FDemo.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 May 14, 2024
@claytercek
Copy link
Author

claytercek commented May 14, 2024 •
edited
Loading

As a workaround for the layer declaration issue, I've added the following to the <head> of my Layout.astro:

<style is:inline>
	@layer reset, component;
</style>

@matthewp
Copy link
Contributor

Thanks, are layers important for this bug report? Could the same sort of issue not exist without the use of layers?

@claytercek
Copy link
Author

Thanks for the quick reply!

Layers are not required for the bug, just used to illustrate the issue.

Per the astro docs, it's recommended that layout components are imported first so that component styles win out if there's conflicting styles with the same specificity.

A common pattern in Astro is to import global CSS inside a Layout component. Be sure to import the Layout component before other imports so that it has the lowest precedence.

@bluwy
Copy link
Member

bluwy commented May 15, 2024

I wonder if it's because we import the layout this way, which messes with the CSS ordering:

astro/packages/integrations/mdx/src/rehype-apply-frontmatter-export.ts

Lines 20 to 30 in 75b93d9

// NOTE(bholmesdev) 08-22-2022
// Using an async layout import (i.e. `const Layout = (await import...)`)
// Preserves the dev server import cache when globbing a large set of MDX files
// Full explanation: 'https://github.com/withastro/astro/pull/4428'
exportNodes.unshift(
jsToTreeNode(
/** @see 'vite-plugin-markdown' for layout props reference */
`import { jsx as layoutJsx } from 'astro/jsx-runtime';
export default async function ({ children }) {
const Layout = (await import(${JSON.stringify(frontmatter.layout)})).default;

@matthewp
Copy link
Contributor

@ascorbic I assigned this to you but you might want to wait until @bholmesdev is back and talk to him about it as I think he has context for why we dynamically import the Layout. Ideally we could stop doing that, and then the import could be at the top.

@bholmesdev
Copy link
Contributor

@matthewp Thanks for tagging me on this! Had to refresh myself with the the PR linked in the code comment. TLDR: an async import prevents dev server slowdowns for layouts that make recursive globs for the page. Using async imports to preserve the Vite cache isn't documented behavior, so it could have changed in the past 2 years.

Unfortunately, this isn't something we have in our testing suite. The linked PR was manually tested against Bryce Wray's blog that displayed the issue.

To repro and test, I would:

  • Create 1000 MDX entries
  • Create a layout that globs for those 1000 MDX entries
  • Apply that layout to the MDX pages
  • Edit one of these MDX entries. See how long the dev server takes to update

If there's still a slowdown when the import is made synchronous, I would keep it where it is.

@Princesseuh Princesseuh added - P3: minor bug An edge case that only affects very specific usage (priority) pkg: mdx Issues pertaining to `@astrojs/mdx` integration and removed needs triage Issue needs to be triaged labels Aug 8, 2024
@bluwy
Copy link
Member

bluwy commented Aug 21, 2024

I'll re-assign this to myself as we've got reports that this blocks the upgrade to @astrojs/mdx v3 (and a v3 specific problem). We'd want to get users upgraded to latest soon so it doesn't break when Astro 5 lands.

@bluwy bluwy assigned bluwy and unassigned ascorbic Aug 21, 2024
@bluwy bluwy mentioned this issue Aug 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bluwy bluwy

Labels
- P3: minor bug An edge case that only affects very specific usage (priority) pkg: mdx Issues pertaining to `@astrojs/mdx` integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix MDX layout style ordering withastro/astro
6 participants
@ascorbic @matthewp @Princesseuh @claytercek @bluwy @bholmesdev