-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Fix image tests crashing #14803
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 image tests crashing #14803
Conversation
This prevents the image tests from crashing. The problem is that there
is a circular reference within the Image component. Because the dev:css
plugin was invalidating the image.css during loading it resulted in the
page never finishing, until it ultimately ran out of memory.
The fix here was to move away from a single global dev:css module and
instead create a virtual:astro:dev:css:${route} module for each of
these.
This fixes the problem in that we don't need to invalidate any more,
each page CSS is represented on its own.
|
|
ematipico
left a comment
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.
Mostly nits, I do have some concerns about using the loader, which should nuke at some point
| entryFileNames: `${settings.config.build.assets}/[name].[hash].js`, | ||
| chunkFileNames: `${settings.config.build.assets}/[name].[hash].js`, | ||
| assetFileNames: `${settings.config.build.assets}/[name].[hash][extname]`, | ||
| ...viteConfig.environments?.client?.build?.rollupOptions?.output, |
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.
What are we fixing/adding here?
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.
This is the script tests you asked about below, this allows people to customize the client-side output names.
| } | ||
|
|
||
| const { css } = await import('virtual:astro:dev-css'); | ||
| const { css } = await loader.import(getDevCSSModuleName(routeData.component)); |
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.
If possible, we shouldn't rely on the loader anymore. We actually want to move away from it eventually, because it goes in conflicts with the new architecture.
In fact, it's possible we won't need this pipeline anymore once we're able to provide all the features using DevApp
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.
Not sure I understand, you can't import Vite code using the real dynamic import. It fails without this change.
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.
That's the concerning part. Why does it work with Cloudflare, and not without it?
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.
Because that code runs inside of the Vite runtime and this does not. So when you use regular dynamic import you are using Node.js module loader which of course can't resolve this. So we use loader to make it load inside the Vite environment.
| import { getVirtualModulePageName } from '../vite-plugin-pages/util.js'; | ||
|
|
||
| const MODULE_DEV_CSS_PREFIX = 'virtual:astro:dev-css:'; | ||
|
|
||
| /** | ||
| * Get the virtual module name for a dev CSS import. | ||
| * Usage: `await loader.import(getDevCSSModuleName(routeData.component))` | ||
| */ | ||
| export function getDevCSSModuleName(componentPath: string): string { | ||
| return getVirtualModulePageName(MODULE_DEV_CSS_PREFIX, componentPath); | ||
| } |
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.
I think this utility should be moved next to its opposite function. Plus, we're repeating the module prefix, while we should instead use one constant to reduce errors
| environments: { | ||
| client: { | ||
| build: { | ||
| rollupOptions: { | ||
| output: { | ||
| entryFileNames: 'assets/entry.[hash].mjs', | ||
| chunkFileNames: 'assets/chunks/chunk.[hash].mjs', | ||
| assetFileNames: 'assets/asset.[hash][extname]', | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
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.
The PR doesn't mention anything about this change. Is it related to the CSS? If so, how does it affect it? If not, what does this change do?
Co-authored-by: Emanuele Stoppa <[email protected]> Co-authored-by: Samuel Macleod <[email protected]> Co-authored-by: Alexander Niebuhr <[email protected]> Co-authored-by: ascorbic <[email protected]> Co-authored-by: matthewp <[email protected]> Co-authored-by: Florian Lefebvre <[email protected]> Co-authored-by: Matt Kane <[email protected]> Co-authored-by: florian-lefebvre <[email protected]> Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Matthew Phillips <[email protected]> Co-authored-by: Matthew Phillips <[email protected]> Co-authored-by: Sarah Rainsberger <[email protected]> Co-authored-by: Erika <[email protected]> Co-authored-by: Hayato Hasegawa <[email protected]> fix: creation of routes list in build (#14325) Fix unit tests for environment API (#14394) fixture (#14599) fix: organize imports and formatting in cloudflare handler (#14697) fix: resolve environment-api build issues for client & CSS (#14770) Fixes CSS when both static/client styles exist (#14790) fix: flag `serverLike` incorrectly computed (#14792) fix: redirects and i18n routing (#14797) fix: logger level and static paths params (#14799) Fix image tests crashing (#14803) fix(environment): Error overlay not working (#14816) fix: hashing of chunks (#14811) Fix before-hydration script emission for environment-api (#14818) fix: node leak into content collections (#14820) fix: avoid `node:path` (#14827) Fix API route tests (#14825) Fix unit tests (#14832) fix: server islands build (#14830) Fix astro-component-bundling tests (#14840) Fix astro-component-bundling tests (#14837) Fix config alias by using resolve.alias config (#14828) fix: container and routing tests (#14852) fix: respect user vite config on sourcemaps (#14847) fix: remove useless tests (#14854) Fixes assetQueryParams and public folder tests (#14853) fix: renderError fallback (#14855) Fix server-islands data being used across tests (#14857) fix: incorrect configuration in tests (#14858) Fixes for tests in environment branch (#14865) Fixes for e2e tests in environment branch (#14870) fix(test): svelte async rendering (#14886) fix(cloudflare/vercel/netlify): some tests pass and excluded others (#14892) Fixes for node integrations tests (#14900) Fix @astrojs/db tests (#14899) fix(environment): Prevent Vite from interpreting injected sourcemap comments as actual sourcemaps (#14921) Fix remaining cloudflare tests (#14912) fix(vite): import mergeConfig from direct file to avoid dynamic import being processed by Vite (#14939) Fix astro:env tests in Cloudflare (#14925)
Changes
This prevents the image tests from crashing. The problem is that there is a circular reference within the Image component. Because the dev:css plugin was invalidating the image.css during loading it resulted in the page never finishing, until it ultimately ran out of memory.
The fix here was to move away from a single global dev:css module and instead create a virtual:astro:dev:css:${route} module for each of these.
This fixes the problem in that we don't need to invalidate any more, each page CSS is represented on its own.
Testing
Docs
N/A, bug fix