feat(astro-mdx): add compiler for @astrojs/mdx to treat layout value from mdx frontmatter as import #1102
Conversation
commit: |
There was a problem hiding this comment.
Thanks for the PR. Seeing this, I'm a bit on the fence when it comes to the matter of adding more complexity to the already "hacky" compilers. This is a "cheap" solution without dependencies, but what's next? Also, can layout be some other user-defined key in the frontmatter without issues?
Just wondering, wouldn't something like https://knip.dev/features/compilers#mdx but with @astrojs/mdx be feasible? If we can support this layout somewhat nicely & safely without users needing to config anything would still be nice, though.
| @@ -28,3 +28,16 @@ export const scriptBodies: SyncCompilerFn = (text: string) => { | |||
| } | |||
| return scripts.join(';\n'); | |||
| }; | |||
|
|
|||
| // Extract paths as imports from frontmatter for given keys (e.g., 'layout') | |||
| const frontmatterMatcher = /---[\s\S]*?---/g; | |||
There was a problem hiding this comment.
This is a greedy match, while there can only be a single frontmatter I think?
There was a problem hiding this comment.
I believe there would only be one. Updated the frontmatter matcher to not be a greedy match.
packages/knip/src/compilers/mdx.ts
Outdated
| let frontmatterImports = ''; | ||
|
|
||
| // If this is an Astro project, also treat the layout path in the frontmatter as an import | ||
| if (mdxDependencies.includes('astro')) { |
There was a problem hiding this comment.
The generic mdxDependencies array contains enablers for the mdx compiler (it's not specific to the project).
There was a problem hiding this comment.
That makes sense. Seems like one option would be to treat layout as a frontmatter for all mdx but that seemed wasteful. Instead I moved AstroMDX as its own compiler and swapped that in the getIncludedCompilers if there was a dependency match.
packages/knip/src/compilers/mdx.ts
Outdated
|
|
||
| // If this is an Astro project, also treat the layout path in the frontmatter as an import | ||
| if (mdxDependencies.includes('astro')) { | ||
| frontmatterImports = importsWithinFrontmatter(text, ['layout']); |
There was a problem hiding this comment.
Feels a bit odd with array/string concatenation. How about working with an array of strings only, just until the end? Empty strings aren't an issue then either.
Or even something like return [...text.replace(), ...importsWithinFrontmatter()]
There was a problem hiding this comment.
Agree, I think working with array of strings until the end is easier to follow.
@webpro I iterated more on this based on the feedback. I think it still feels mostly "hacky". I will feel no real loss if this doesn't feel like the right direction to solve this. At least if it's wrong it gives a better picture of what right might end up looking like( or not looking like). |
Co-authored-by: Lars Kappert <[email protected]>
cylewaitforit
changed the title
|
🚀 This pull request is included in v5.59.0. See Release 5.59.0 for release notes. Using Knip in a commercial project? Please consider becoming a sponsor. |
Treats the layout in the mdx frontmatter in an Astro project as an import.
Related Issue: #1084