-
Notifications
You must be signed in to change notification settings - Fork 336
fix(cli): don't rewrite @/ imports inside MDX code blocks #17406
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| - summary: | | ||
| Fix `@/`-prefixed imports inside fenced code blocks and inline code being rewritten to relative | ||
| paths. Code samples are now rendered verbatim; only real MDX imports are resolved. | ||
| type: fix |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,58 @@ | ||||||||||||||||||||||||||||||||||||||
| import { AbsoluteFilePath, dirname, RelativeFilePath, relative } from "@fern-api/fs-utils"; | ||||||||||||||||||||||||||||||||||||||
| import grayMatter from "gray-matter"; | ||||||||||||||||||||||||||||||||||||||
| import { CONTINUE, visit } from "unist-util-visit"; | ||||||||||||||||||||||||||||||||||||||
| import { parseMarkdownToTree } from "./parseMarkdownToTree.js"; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||
| * Match import statements with '@/' prefix | ||||||||||||||||||||||||||||||||||||||
| * Handles various import formats: | ||||||||||||||||||||||||||||||||||||||
| * - import { X } from '@/path' | ||||||||||||||||||||||||||||||||||||||
| * - import X from '@/path' | ||||||||||||||||||||||||||||||||||||||
| * - import * as X from '@/path' | ||||||||||||||||||||||||||||||||||||||
| * - import '@/path' | ||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||
| const IMPORT_REGEX = /(import\s+(?:(?:\{[^}]*\}|\*\s+as\s+\w+|\w+)\s+from\s+)?['"])@\/([^'"]+)(['"])/g; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| interface Range { | ||||||||||||||||||||||||||||||||||||||
| start: number; | ||||||||||||||||||||||||||||||||||||||
| end: number; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||
| * Byte ranges of fenced code blocks and inline code spans. Import statements inside them are | ||||||||||||||||||||||||||||||||||||||
| * documentation samples, not MDX imports, and must be rendered verbatim. | ||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||
| function getCodeRanges(markdown: string): Range[] | undefined { | ||||||||||||||||||||||||||||||||||||||
| const { content } = grayMatter(markdown); | ||||||||||||||||||||||||||||||||||||||
| // `parseMarkdownToTree` strips frontmatter, so node offsets are relative to the body. | ||||||||||||||||||||||||||||||||||||||
| if (!markdown.endsWith(content)) { | ||||||||||||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| const bodyOffset = markdown.length - content.length; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||
| const tree = parseMarkdownToTree(markdown); | ||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 suggestion
|
||||||||||||||||||||||||||||||||||||||
| const ranges: Range[] = []; | ||||||||||||||||||||||||||||||||||||||
| visit( | ||||||||||||||||||||||||||||||||||||||
| tree, | ||||||||||||||||||||||||||||||||||||||
| (node: unknown) => { | ||||||||||||||||||||||||||||||||||||||
| const type = (node as { type?: string }).type; | ||||||||||||||||||||||||||||||||||||||
| return type === "code" || type === "inlineCode"; | ||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+36
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 suggestion
Suggested change
Comment on lines
+36
to
+41
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Type assertion used where the repository forbids it The node passed to the traversal check is force-cast to a shape ( REVIEW.md: "No `as X` type assertions unless the compiler genuinely cannot infer the type"The assertion is only needed because the predicate parameter is annotated
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||||||||||||||||||
| (node) => { | ||||||||||||||||||||||||||||||||||||||
| const start = node.position?.start.offset; | ||||||||||||||||||||||||||||||||||||||
| const end = node.position?.end.offset; | ||||||||||||||||||||||||||||||||||||||
| if (start != null && end != null) { | ||||||||||||||||||||||||||||||||||||||
| ranges.push({ start: start + bodyOffset, end: end + bodyOffset }); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| return CONTINUE; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| return ranges; | ||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+52
to
+54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Parsing failures are swallowed with no logging Errors from parsing the page are discarded by an empty catch block ( Empty catch violates REVIEW.md TypeScript conventionREVIEW.md states: "No empty Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||
| * Transforms import statements with '@/' prefix to relative paths. | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -25,28 +79,32 @@ export function transformAtPrefixImports({ | |||||||||||||||||||||||||||||||||||||
| absolutePathToFernFolder: AbsoluteFilePath; | ||||||||||||||||||||||||||||||||||||||
| absolutePathToMarkdownFile: AbsoluteFilePath; | ||||||||||||||||||||||||||||||||||||||
| }): string { | ||||||||||||||||||||||||||||||||||||||
| // Match import statements with '@/' prefix | ||||||||||||||||||||||||||||||||||||||
| // Handles various import formats: | ||||||||||||||||||||||||||||||||||||||
| // - import { X } from '@/path' | ||||||||||||||||||||||||||||||||||||||
| // - import X from '@/path' | ||||||||||||||||||||||||||||||||||||||
| // - import * as X from '@/path' | ||||||||||||||||||||||||||||||||||||||
| // - import '@/path' | ||||||||||||||||||||||||||||||||||||||
| const importRegex = /(import\s+(?:(?:\{[^}]*\}|\*\s+as\s+\w+|\w+)\s+from\s+)?['"])@\/([^'"]+)(['"])/g; | ||||||||||||||||||||||||||||||||||||||
| if (!markdown.includes("@/")) { | ||||||||||||||||||||||||||||||||||||||
| return markdown; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const mdxDir = dirname(absolutePathToMarkdownFile); | ||||||||||||||||||||||||||||||||||||||
| const codeRanges = getCodeRanges(markdown); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| return markdown.replace(importRegex, (match, prefix, importPath, suffix) => { | ||||||||||||||||||||||||||||||||||||||
| // Compute the absolute path of the imported file (from fern folder root) | ||||||||||||||||||||||||||||||||||||||
| const absoluteImportPath = AbsoluteFilePath.of(`${absolutePathToFernFolder}/${importPath}`); | ||||||||||||||||||||||||||||||||||||||
| return markdown.replace( | ||||||||||||||||||||||||||||||||||||||
| IMPORT_REGEX, | ||||||||||||||||||||||||||||||||||||||
| (match, prefix: string, importPath: string, suffix: string, offset: number) => { | ||||||||||||||||||||||||||||||||||||||
| if (codeRanges?.some((range) => offset >= range.start && offset < range.end)) { | ||||||||||||||||||||||||||||||||||||||
| return match; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Compute the relative path from the MDX file's directory to the imported file | ||||||||||||||||||||||||||||||||||||||
| let relativePath = relative(mdxDir, absoluteImportPath); | ||||||||||||||||||||||||||||||||||||||
| // Compute the absolute path of the imported file (from fern folder root) | ||||||||||||||||||||||||||||||||||||||
| const absoluteImportPath = AbsoluteFilePath.of(`${absolutePathToFernFolder}/${importPath}`); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Ensure the path starts with './' or '../' for proper module resolution | ||||||||||||||||||||||||||||||||||||||
| if (!relativePath.startsWith(".") && !relativePath.startsWith("/")) { | ||||||||||||||||||||||||||||||||||||||
| relativePath = RelativeFilePath.of(`./${relativePath}`); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| // Compute the relative path from the MDX file's directory to the imported file | ||||||||||||||||||||||||||||||||||||||
| let relativePath = relative(mdxDir, absoluteImportPath); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Ensure the path starts with './' or '../' for proper module resolution | ||||||||||||||||||||||||||||||||||||||
| if (!relativePath.startsWith(".") && !relativePath.startsWith("/")) { | ||||||||||||||||||||||||||||||||||||||
| relativePath = RelativeFilePath.of(`./${relativePath}`); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| return `${prefix}${relativePath}${suffix}`; | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
| return `${prefix}${relativePath}${suffix}`; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
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.
🟡 warning
grayMatter()sits outside thetry, so malformed YAML frontmatter now throws out oftransformAtPrefixImports— a path that previously never parsed anything and couldn't fail. Given this runs on publish (DocsDefinitionResolver,publishDocs), a broken frontmatter block would go from "imports rewritten anyway" to "build blows up". Move it inside the guard.