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

feat: Expose $entry variable to Markdoc #6588

Merged
merged 21 commits into from Mar 21, 2023
Merged

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented Mar 17, 2023

Changes

  • Expose content collection entry information to Markdoc files. This allows you to render the parsed frontmatter based on your schema, alongside the generated id, slug, and collection name.
---
title: Hello Markdoc!
---

# {% $entry.data.title %}
  • Introduce a new getRenderModule() helper on our internal addContentEntryType API. This exposes the parsed entry to your Vite loader for use while generating the module (ex. to expose an $entry variable to the Markdoc transformer). This also standardizes how the result of .render() is generated. Before, integration authors would need to wire up a separate Vite plugin that resolves modules of your preferred extension and hope this meshes with Astro's content collection API (see diff here). By standardizing this, we can change how and when rendered modules are loaded in the future without breaking changes.
  • Introduce a caching layer for entry info, so this information can be passed to getRenderModule().

Testing

  • Add Markdoc integration test for using the $entry variable

Docs

  • README section on $entry

@bholmesdev bholmesdev requested a review from a team as a code owner March 17, 2023 22:59
@changeset-bot
Copy link

changeset-bot bot commented Mar 17, 2023

🦋 Changeset detected

Latest commit: 1071319

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: integration Related to any renderer integration (scope) labels Mar 17, 2023
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.

LGTM! For a second I thought it's a wordplay on Sentry 🙈

Copy link
Member

@yanthomasdev yanthomasdev left a comment

Choose a reason for hiding this comment

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

Good job @bholmesdev, docs LGTM

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I admit that I'm struggling to understand the function setContentEntryModuleCache. It does a lot of stuff. Of course, I don't have much knowledge of the tool yet, so I lack a lot of contexts.

packages/astro/src/content/vite-plugin-content-imports.ts Outdated Show resolved Hide resolved
packages/astro/src/content/vite-plugin-content-imports.ts Outdated Show resolved Hide resolved
packages/astro/src/content/vite-plugin-content-imports.ts Outdated Show resolved Hide resolved
packages/astro/src/content/vite-plugin-content-imports.ts Outdated Show resolved Hide resolved
fileId: string;
pluginContext: PluginContext;
}): Promise<ContentEntryModule> {
contentEntryModuleByIdCache.set(fileId, 'loading');
Copy link
Member

Choose a reason for hiding this comment

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

I admit that having a "loading" value inside a cache is not something you see often. Usually, you either have or do not have value. Would you mind explaining how this caching works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, sorry for the unconventional implementation! Trying to track 3 possible states here:

  1. Module has not been cached yet (undefined)
  2. Module is being resolved, but the async operation hasn't completed yet ('loading')
  3. Module has been resolved and cached (ContentEntryModule)

This gets around an annoying problem I hit in production builds, where Vite tries to load a content entry's rendered content before its frontmatter module has been resolved. By tracking which cached entries haven't resolved yet with 'loading', we can queue up a promise to "wait" for that frontmatter module to be resolved before rendering the post's content module. I tried implementing this with cached promises instead, but truthfully couldn't wrap my head around the complexity for a friday evening 😅

If this is too confusing, I'm happy to refactor. Let me know if there's an implementation I'm not seeing

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the thorough explanation! I understand now because we have unconventional implementation. Maybe it's worth documenting it with a comment, so other developers will know why we have this caching system.

I think the best approach is to return promises from the cache if we know that a value is always returned when the promise is fulfilled. Undoubtedly the implementation is trickier, but it allows us to use just two states, without the burden of maintaining three states.

Although, I understand if the implementation becomes trickier.

If I would suggest a solution, maybe wrapping the whole function into a new Promise should be enough. Instead of return value we do resolve(value)

packages/astro/src/content/vite-plugin-content-imports.ts Outdated Show resolved Hide resolved
fileId: string;
pluginContext: PluginContext;
}): Promise<ContentEntryModule> {
contentEntryModuleByIdCache.set(fileId, 'loading');
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the thorough explanation! I understand now because we have unconventional implementation. Maybe it's worth documenting it with a comment, so other developers will know why we have this caching system.

I think the best approach is to return promises from the cache if we know that a value is always returned when the promise is fulfilled. Undoubtedly the implementation is trickier, but it allows us to use just two states, without the burden of maintaining three states.

Although, I understand if the implementation becomes trickier.

If I would suggest a solution, maybe wrapping the whole function into a new Promise should be enough. Instead of return value we do resolve(value)

@bholmesdev
Copy link
Contributor Author

@ematipico Alright, rewrote the cache implementation and added a thorough code comment. It's not quite the suggestion you made (missed that while I was working) but I think this puts us in a better spot. Opening for re-review if you want to take a look! We can refine from here in a follow-up PR as well.

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

The implementation is way better now! Thank you for taking the time for refactoring it!

@bholmesdev bholmesdev merged commit f42f47d into main Mar 21, 2023
15 checks passed
@bholmesdev bholmesdev deleted the feat/markdoc-entry-data branch March 21, 2023 13:20
@astrobot-houston astrobot-houston mentioned this pull request Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants