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

Fix duplicated Astro and Vite injected styles #8706

Merged
merged 2 commits into from
Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/olive-bags-think.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fix duplicated Astro and Vite injected styles
36 changes: 0 additions & 36 deletions packages/astro/e2e/css-sourcemaps.test.js

This file was deleted.

18 changes: 3 additions & 15 deletions packages/astro/e2e/css.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,9 @@ test.describe('CSS HMR', () => {
await expect(h).toHaveCSS('color', 'rgb(0, 128, 0)');
});

test('removes Astro-injected CSS once Vite-injected CSS loads', async ({ page, astro }) => {
test('removes Astro-injected CSS once Vite-injected CSS loads', async ({ astro }) => {
const html = await astro.fetch('/').then((res) => res.text());

// style[data-astro-dev-id] should exist in initial SSR'd markup
expect(html).toMatch('data-astro-dev-id');

await page.goto(astro.resolveUrl('/'));

// Ensure JS has initialized
await page.waitForTimeout(500);

// style[data-astro-dev-id] should NOT exist once JS runs
expect(await page.locator('style[data-astro-dev-id]').count()).toEqual(0);

// style[data-vite-dev-id] should exist now
expect(await page.locator('style[data-vite-dev-id]').count()).toBeGreaterThan(0);
// style[data-vite-dev-id] should exist in initial SSR'd markup
expect(html).toMatch('data-vite-dev-id');
});
});
7 changes: 0 additions & 7 deletions packages/astro/e2e/fixtures/css-sourcemaps/astro.config.mjs

This file was deleted.

8 changes: 0 additions & 8 deletions packages/astro/e2e/fixtures/css-sourcemaps/package.json

This file was deleted.

1 change: 0 additions & 1 deletion packages/astro/e2e/fixtures/css-sourcemaps/src/env.d.ts

This file was deleted.

This file was deleted.

This file was deleted.

2 changes: 1 addition & 1 deletion packages/astro/e2e/view-transitions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ test.describe('View Transitions', () => {
});

test('client:only styles are retained on transition', async ({ page, astro }) => {
const totalExpectedStyles = 8;
const totalExpectedStyles = 7;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no more <style data-astro-dev-id as it's merged into <style data-vite-dev-id, so this number is reduced by one.


// Go to page 1
await page.goto(astro.resolveUrl('/client-only-one'));
Expand Down
45 changes: 3 additions & 42 deletions packages/astro/src/runtime/client/hmr.ts
Original file line number Diff line number Diff line change
@@ -1,46 +1,7 @@
/// <reference types="vite/client" />

if (import.meta.hot) {
// Vite injects `<style data-vite-dev-id>` for ESM imports of styles
// but Astro also SSRs with `<style data-astro-dev-id>` blocks. This MutationObserver
// removes any duplicates as soon as they are hydrated client-side.
const injectedStyles = getInjectedStyles();
const mo = new MutationObserver((records) => {
for (const record of records) {
for (const node of record.addedNodes) {
if (isViteInjectedStyle(node)) {
injectedStyles.get(node.getAttribute('data-vite-dev-id')!)?.remove();
}
}
}
});
mo.observe(document.documentElement, { subtree: true, childList: true });

// Vue `link` styles need to be manually refreshed in Firefox
import.meta.hot.on('vite:beforeUpdate', async (payload) => {
Comment on lines -19 to -20
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I manually tested this and it seems to not apply anymore, so I removed it. The rest of this file relates to our CSS handling, which is no longer needed as Vite handles all now, so pretty much the entire file is empty and not need to be loaded anymore.

for (const file of payload.updates) {
if (file.acceptedPath.includes('vue&type=style')) {
const link = document.querySelector(`link[href="${file.acceptedPath}"]`);
if (link) {
link.replaceWith(link.cloneNode(true));
}
}
}
});
}

function getInjectedStyles() {
const injectedStyles = new Map<string, Element>();
document.querySelectorAll<HTMLStyleElement>('style[data-astro-dev-id]').forEach((el) => {
injectedStyles.set(el.getAttribute('data-astro-dev-id')!, el);
});
return injectedStyles;
}

function isStyle(node: Node): node is HTMLStyleElement {
return node.nodeType === node.ELEMENT_NODE && (node as Element).tagName.toLowerCase() === 'style';
}

function isViteInjectedStyle(node: Node): node is HTMLStyleElement {
return isStyle(node) && !!node.getAttribute('data-vite-dev-id');
// HMR temporarily not needed for now, but kept here in case we need it again.
// To re-instate this module again, update `vite-plugin-astro-server/route.ts`
// to add this module as a script similar to `/@vite/client`
}
2 changes: 1 addition & 1 deletion packages/astro/src/transitions/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ async function updateDOM(
const devId = el.dataset.viteDevId;
// If this same style tag exists, remove it from the new page
return (
newDocument.querySelector(`style[data-astro-dev-id="${devId}"]`) ||
newDocument.querySelector(`style[data-vite-dev-id="${devId}"]`) ||
// Otherwise, keep it anyways. This is client:only styles.
noopEl
);
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/vite-plugin-astro-server/css.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export async function getStylesForURL(
mode === 'development' && // only inline in development
typeof ssrModule?.default === 'string' // ignore JS module styles
) {
importedStylesMap.set(importedModule.url, ssrModule.default);
importedStylesMap.set(importedModule.id ?? importedModule.url, ssrModule.default);
} else {
// NOTE: We use the `url` property here. `id` would break Windows.
importedCssUrls.add(importedModule.url);
Expand Down
15 changes: 4 additions & 11 deletions packages/astro/src/vite-plugin-astro-server/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { loadMiddleware } from '../core/middleware/loadMiddleware.js';
import { createRenderContext, getParamsAndProps, type SSROptions } from '../core/render/index.js';
import { createRequest } from '../core/request.js';
import { matchAllRoutes } from '../core/routing/index.js';
import { isPage, resolveIdToUrl, viteID } from '../core/util.js';
import { isPage } from '../core/util.js';
import { getSortedPreloadedMatches } from '../prerender/routing.js';
import { isServerLikeOutput } from '../prerender/utils.js';
import { PAGE_SCRIPT_ID } from '../vite-plugin-scripts/index.js';
Expand Down Expand Up @@ -275,13 +275,6 @@ async function getScriptsAndStyles({ pipeline, filePath }: GetScriptsAndStylesPa
props: { type: 'module', src: '/@vite/client' },
children: '',
});
scripts.add({
props: {
type: 'module',
src: await resolveIdToUrl(moduleLoader, 'astro/runtime/client/hmr.js'),
},
children: '',
});
}

// TODO: We should allow adding generic HTML elements to the head, not just scripts
Expand Down Expand Up @@ -322,11 +315,11 @@ async function getScriptsAndStyles({ pipeline, filePath }: GetScriptsAndStylesPa
},
children: '',
});
// But we still want to inject the styles to avoid FOUC
// But we still want to inject the styles to avoid FOUC. The style tags
// should emulate what Vite injects so further HMR works as expected.
styles.add({
props: {
// Track the ID so we can match it to Vite's injected style later
'data-astro-dev-id': viteID(new URL(`.${url}`, settings.config.root)),
'data-vite-dev-id': url,
},
children: content,
});
Expand Down