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

[Markdoc] Fix global asset bleed #6758

Merged
merged 24 commits into from
May 22, 2023
Merged

[Markdoc] Fix global asset bleed #6758

merged 24 commits into from
May 22, 2023

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented Apr 5, 2023 •
edited
Loading

Changes

  • Fix script and style bleed in Markdoc when using Astro component tags. This was introduced by the new markdoc.config.mjs in version 0.1.0.
    • Inject the astroPropagatedAssets on all Astro component imports
    • Handle propagated assets from the Markdoc renderer
  • Add a handlePropagation option for content collection integrations. This allows us to toggle asset propagation wrappers on or off by file extension. Ex. MDX needs this wrapper, but Markdown and Markdoc do not.

Testing

TODO

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Apr 5, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 96e470b

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 github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) pkg: integration Related to any renderer integration (scope) labels Apr 5, 2023
@bholmesdev bholmesdev force-pushed the fix/markdoc-global-assets branch from 4443690 to df3e433 Compare April 26, 2023 21:16
@bholmesdev bholmesdev changed the title [Draft][Markdoc] Fix global asset bleed [Markdoc] Fix global asset bleed Apr 26, 2023
@matthewp matthewp force-pushed the fix/markdoc-global-assets branch from 672639a to 4b18f42 Compare May 17, 2023 17:04
@matthewp matthewp marked this pull request as ready for review May 19, 2023 20:32
@matthewp matthewp requested a review from a team as a code owner May 19, 2023 20:32
delucis
delucis approved these changes May 19, 2023
View reviewed changes
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Seeing a review request, but I don’t think this needs a docs maintainers review? This is mostly invisible to users and just a bug fix right? Nice work, Ben!

bluwy
bluwy reviewed May 22, 2023
View reviewed changes
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Only one question, other than that it looks good to me. Propagation stuff is quite magical to me 😄

packages/astro/src/core/render/dev/vite.ts
@@ -10,9 +9,10 @@ import { isCSSRequest } from './util.js';
* List of file extensions signalling we can (and should) SSR ahead-of-time
* See usage below
*/
const fileExtensionsToSSR = new Set(['.astro', ...SUPPORTED_MARKDOWN_FILE_EXTENSIONS]);
const fileExtensionsToSSR = new Set(['.astro', '.mdoc', ...SUPPORTED_MARKDOWN_FILE_EXTENSIONS]);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we're hardcoding .mdoc here?

Copy link
Contributor

Choose a reason for hiding this comment

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

These are modules that we need to ensure are imported before we do rendering in dev. We need to do this so that we can discover which CSS to include on the page.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I was hoping there's a way the markdoc integration could tell about this information so we don't have to hardcode this, but if it's not easy to do so, I'm usually fine with things like this 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I mean it definitely could. We need a generic solution if we keep going down that path. Maybe using metadata or something like that... probably would rather hold off until the next time to tackle that.

@matthewp matthewp merged commit f558a9e into main May 22, 2023
@matthewp matthewp deleted the fix/markdoc-global-assets branch May 22, 2023 17:50
@astrobot-houston astrobot-houston mentioned this pull request May 22, 2023
Merged
@bholmesdev bholmesdev mentioned this pull request May 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewp matthewp matthewp left review comments

@delucis delucis delucis approved these changes

@bluwy bluwy bluwy approved these changes

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bholmesdev @delucis @matthewp @bluwy