fix(markdown): Fix Markdown images breaking the build #8598
Conversation
🦋 Changeset detectedLatest commit: e095d44 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 |
github-actions
bot
added
the
pkg: astro
Co-authored-by: Sarah Rainsberger <[email protected]>
| ...images[imagePath].attributes, | ||
| }) | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| const html = updateImageReferences(${JSON.stringify(html)}); |
There was a problem hiding this comment.
Since updateImageReferences now returns a promise, wouldn't html become a promise and affect compiledContent and the Content render code?
Seems like we need another TLA here but maybe that is able to side-step the Rollup bug.
There was a problem hiding this comment.
Ah yes, I didn't notice that it made html a promise since it worked in rendering... Unfortunately, putting a TLA anywhere in this file causes the issue. No idea what to do then.
There was a problem hiding this comment.
I think the Rollup bug is only caused when dynamically importing a module that causes an import loop, but this PR removed the dynamic import as a top-level import, so theoretically, I think we can keep using TLA?
Unless if you're already tested it, then pardon me 😅 We might have to refactor some modules to break the loop otherwise. Or fix in Rollup directly.
There was a problem hiding this comment.
By TLA, I mean directly on this line, like const html = await ...
There was a problem hiding this comment.
Doing const html = await causes the issue, unfortunately
There was a problem hiding this comment.
I guess I could try fixing this in Rollup directly
|
I think my last commit should work as a workaround for now. The Rollup chunking algorithm is amazing but I can't figure out how to handle TLA deadlock specifically. It already failed to chunk correctly on the initial chunk-by-module-dependants flow. |
| // https://github.com/rollup/rollup/issues/4708 | ||
| extendManualChunks(outputOptions, { | ||
| before(id) { | ||
| if (id.includes('astro/dist/assets/services')) { |
There was a problem hiding this comment.
Will this hardcoding cause an issue for custom services?
There was a problem hiding this comment.
Would custom services import things like from astro/assets/utils? Then maybe we need to go one directory up and make it astro/dist/assets instead 🤔
There was a problem hiding this comment.
Hmm, admittedly, it's fairly unlikely
There was a problem hiding this comment.
I think either ways it should still work fine as we're primarily grouping our Astro code into one chunk. The issue before was that services/sharp.ts uses services/service.ts. But both were in two different chunks. services/sharp.ts was in it's own chunk, but services/service.ts got grouped with the markdown page chunk. And there was an import loop after Rollup chunks it, and the TLA in the markdown page made it worst.
So manually handling where services/service.ts should be chunked fixed it here. I don't mind if we want to go back one directory down though.
There was a problem hiding this comment.
I'm honestly not sure of the impact this can have, so I think whichever works just fine for me as long as it works, ha.
If it turns out to be a problem somehow, we can change it back fairly easily it seems like
There was a problem hiding this comment.
Changed it back to astro/dist/assets/services/ for now since it feels a liitle safer that way 👍
Changes
This PR reworks how we handle images in Markdown to workaround a Rollup bug with top level await. This has the side effect of needing us to rework how error handling is done around these parts, which honestly is great because it had some Markdown specific parts that I was able to make more general, improving the overall experience.
Fix #8547
Also fix #8454, by accident
Testing
Tests should all pass! The specific problem here is kinda hard to test because it depends specifically on how the website gets bundled, confusing
Docs
Added two errors and deprecated an old one. One of added one is mostly a copy paste of the deprecated one.