-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Fixes for tests in environment branch #14865
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
Conversation
🦋 Changeset detectedLatest commit: b7eeccb The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
If the module with the server island mapping is in a chunk, need to use a parent-relative import statement to reach the server island module itself.
Properly handle isDev in the virtual module
Prod now matches dev, we default to adding a trailing slash to the Astro.request.url. Improves an annoying dev/build different in Astro 5.
Propagated styles get embedded in the JS and rendered at runtime if they are used on a page. These need to include the base, which this change makes happen.
Princesseuh
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.
Looks great as far as I can tell!
| // Delete empty CSS chunks. In prerender these are likely duplicates | ||
| // from SSR. | ||
| if(stylesheet.source.length === 0) { | ||
| delete bundle[id]; | ||
| return; | ||
| } |
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.
Shouldn't we check that we're inside the prerender pipeline?
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 should be ok. Should we leave in empty CSS chunks in the other environments?
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 minor issue I see is that the comment doesn't reflect the actual code, because I can't see anything regarding "SSR" and "prerender" , so I'm not sure how to interpret the code.
|
|
||
| export function mergeInlineCss( | ||
| acc: Array<StylesheetAsset>, | ||
| current: StylesheetAsset, | ||
| ): Array<StylesheetAsset> { |
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.
Comment missing, we should explain the business logic of the function
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 function already existed, it just got moved from internal.ts because it's now needed at runtime after the build pipeline refactor.
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 understand that it already exists, but that doesn't mean we shouldn't improve what we already have. For example, I don't know what its business logic is.
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.
Added a comment on what this does.
| for (const imp of imported) { | ||
| if (imp.id && !seen.has(imp?.id)) { | ||
| yield* collectCSSWithOrder(imp.id, imp, seen); | ||
| } | ||
| } |
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.
Nit: I think if we avoid the recursion and we just use a simple loop, it will get faster (debatable) and use less memory
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 same way getStylesForURL works in main and has always worked. Crawling a graph is a very natural use-case for recursion. I have no doubt it can be done faster and am not opposed to doing so, but this branch is for fixing bugs and this is not any slower than what we currently do in Astro 5.
| const isRelativeChunk = !chunk.isEntry; | ||
| const dots = isRelativeChunk ? '..' : '.'; |
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'm not sure why we needed to add this check. fileName is created by rollup, and referenceId is created using rollup APIs.
If there's a bug, it should probably be on lines 73-78, an incorrect usage of the rollup apis, or a bug in rollup.
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.
It's needed when the module that contains the server island manifest and the server island chunks are both in the chunks folder.
chunks/
server-island-manifest.hash.mjs
some-server-island.hash.mjs
fileName here would be chunks/some-server-island.hash.mjs, so the relative import would be wrong import('./chunks/some-server-island.hash.mjs.
Ideally we could avoid writing this at all and not need to emit chunks ourselves. I think that might require adding another environment though, so a larger task than we have time for at the moment.
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.
And in which case we need the ../?
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.
And in which case we need the ../?
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
Testing
test/ssr-manifest.test.js- 4578d2dtest/content-collections.test.js- 5fd8edftest/config-vite.test.js- 9c3aa38test/entry-file-names.test.js- 4509536test/redirects.test.js- 11ab96etest/server-islands.test.js- 15fc25etest/css-inline-stylesheets.test.js- 8897c3etest/css-import-as-inline.test.js- de91192test/vitest.test.js- d5b6a76test/serializeManifest.test.js- 7794452test/ssr-dynamic.test.js- 59cd518test/core-image-unconventional-settings.test.js- ce5ed6btest/0-css.test.js- ad982c6test/astro-global.test.js- 732cfaftest/content-collections.test.js- 1bedc36Docs
N/A, bug fix