Conversation
How to use the Graphite Merge QueueAdd the label graphite: merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment was marked as outdated.
This comment was marked as outdated.
sxzz
force-pushed
the
docs/reference
branch
2 times, most recently
from
188bb33 to
8420d58
Compare
|
I squashed the commit and now triggered. |
|
For Suggestions:
|
|
Ah I see. You are gonna remove the |
|
Yes. We could do this.
Hmm. I think having a If you put it only in the nav bar. I think people will confuse about the relationship between
This comes from vite's doc. https://vite.dev/config/ |
|
This is a bit different from Vite. Essentially, Config is just one item in the sidebar, whereas Vite dedicates several pages to explaining config in detail. Also, I'm not sure what approach the Rolldown team wants to take for introducing config, whether we plan to provide both a reference and handwritten guidance, or choose just one style. |
hyf0
left a comment
hyf0
left a comment
There was a problem hiding this comment.
LGTM. Sill let's see if there are opinions from @TheAlexLichter.
There was a problem hiding this comment.
Pull Request Overview
This PR adds automated API reference documentation generation using TypeDoc. The documentation is generated from TypeScript source files and integrated into the VitePress documentation site.
Key changes:
- Added TypeDoc-based API reference generation with VitePress theme integration
- Updated documentation navigation to link to generated reference pages instead of manual docs
- Added pnpm trust policy exclusions for specific dependency versions
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated packageManager to [email protected] and modified docs:build to include generation step |
| pnpm-workspace.yaml | Added trustPolicyExclude entries for semver, chokidar, and rxjs |
| pnpm-lock.yaml | Added TypeDoc dependencies and updated metadata (libc fields, version bumps) |
| knip.jsonc | Added typedoc-vitepress-theme to ignoreDependencies for docs workspace |
| docs/package.json | Added TypeDoc dependencies and generate script |
| docs/apis/bundler-api.md | Removed placeholder documentation file |
| docs/apis/plugin-api.md | Updated to reference new generated API documentation |
| docs/.vitepress/scripts/generate-reference.ts | New script to generate API reference using TypeDoc |
| docs/.vitepress/config.ts | Restructured sidebar to integrate generated TypeDoc documentation |
| .gitignore | Added docs/reference to ignore generated documentation |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await rm('reference/api/index.md', { force: true }); | ||
| await rm('reference/api/_media', { recursive: true, force: true }); | ||
|
|
||
| /** | ||
| * Run TypeDoc with the specified tsconfig | ||
| */ | ||
| async function runTypedoc(): Promise<void> { | ||
| const root = path.resolve( | ||
| import.meta.dirname, | ||
| '../../..', | ||
| ); | ||
|
|
||
| const options: TypeDocOptions & PluginOptions = { | ||
| tsconfig: path.resolve( | ||
| root, | ||
| 'packages/rolldown/tsconfig.json', | ||
| ), | ||
| plugin: ['typedoc-plugin-markdown', 'typedoc-vitepress-theme'], | ||
| out: './reference', | ||
| entryPoints: [ | ||
| path.resolve(root, 'packages/rolldown/src/index.ts'), | ||
| ], | ||
| excludeInternal: true, | ||
|
|
||
| hideBreadcrumbs: true, | ||
| useCodeBlocks: true, | ||
| flattenOutputFiles: true, | ||
|
|
||
| // @ts-expect-error VitePress config | ||
| docsRoot: './reference', | ||
| }; | ||
| const app = await Application.bootstrapWithPlugins(options); |
There was a problem hiding this comment.
[nitpick] The out option is set to './reference', which is relative to the current working directory. Since this script is located in docs/.vitepress/scripts/, when executed, the current working directory will be docs/ (based on the package.json location).
However, the cleanup operations on lines 12-13 use paths like 'reference/api/index.md' which are also relative to the current working directory. This should work correctly, but it would be clearer and more robust to use path.resolve() consistently:
const docsRoot = path.resolve(import.meta.dirname, '../..');
await rm(path.resolve(docsRoot, 'reference/api/index.md'), { force: true });
await rm(path.resolve(docsRoot, 'reference/api/_media'), { recursive: true, force: true });And update the options:
out: path.resolve(docsRoot, 'reference'),| })).sort((a) => { | ||
| if (a.text === 'Functions') return -1; |
There was a problem hiding this comment.
The sort function has incorrect logic. Currently, it only returns -1 when a.text === 'Functions', but doesn't return a value for the else case. This means items that aren't "Functions" will have undefined as their sort value, which can lead to unpredictable sorting behavior.
The function should return 1 (or a positive number) for non-Functions items to ensure proper sorting:
})).sort((a) => {
if (a.text === 'Functions') return -1;
return 1;
});Or use a proper comparator with both parameters:
})).sort((a, b) => {
if (a.text === 'Functions') return -1;
if (b.text === 'Functions') return 1;
return 0;
});| })).sort((a) => { | |
| if (a.text === 'Functions') return -1; | |
| })).sort((a, b) => { | |
| if (a.text === 'Functions') return -1; | |
| if (b.text === 'Functions') return 1; |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| })).sort((a) => { | ||
| if (a.text === 'Functions') return -1; | ||
| return 0; | ||
| }); |
There was a problem hiding this comment.
The sort comparison function is incorrect and will not sort properly. The sort() method requires a comparator function with two parameters (a, b), but only one parameter is provided.
// Current (broken)
}).sort((a) => {
if (a.text === 'Functions') return -1;
return 0;
});
// Fixed
}).sort((a, b) => {
if (a.text === 'Functions') return -1;
if (b.text === 'Functions') return 1;
return 0;
});Without comparing both elements, the sort will produce inconsistent results and may not place 'Functions' first as intended.
| })).sort((a) => { | |
| if (a.text === 'Functions') return -1; | |
| return 0; | |
| }); | |
| })).sort((a, b) => { | |
| if (a.text === 'Functions') return -1; | |
| if (b.text === 'Functions') return 1; | |
| return 0; | |
| }); |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| console.log('📚 Beautifying reference structure...'); | ||
|
|
||
| await rm('reference/api/index.md', { force: true }); | ||
| await rm('reference/api/_media', { recursive: true, force: true }); |
There was a problem hiding this comment.
Is typedoc supposed to create a _media file?
There was a problem hiding this comment.
It might be necessary if there are external documents involved. For more information, see: https://typedoc.org/documents/External_Documents.html
I have removed this line.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function getTypedocSidebar() { | ||
| const filepath = path.resolve( | ||
| import.meta.dirname, | ||
| '../reference/typedoc-sidebar.json', | ||
| ); | ||
| if (!existsSync(filepath)) return []; | ||
|
|
||
| try { | ||
| return require(filepath) as DefaultTheme.SidebarItem[]; | ||
| } catch (error) { | ||
| console.error('Failed to load typedoc sidebar:', error); | ||
| return []; |
There was a problem hiding this comment.
Consider caching the TypeDoc sidebar to avoid loading it from the filesystem on every config access. Since VitePress config is evaluated multiple times during development, this could lead to unnecessary file system operations. You could use a let variable outside the function to cache the result.
| function getTypedocSidebar() { | |
| const filepath = path.resolve( | |
| import.meta.dirname, | |
| '../reference/typedoc-sidebar.json', | |
| ); | |
| if (!existsSync(filepath)) return []; | |
| try { | |
| return require(filepath) as DefaultTheme.SidebarItem[]; | |
| } catch (error) { | |
| console.error('Failed to load typedoc sidebar:', error); | |
| return []; | |
| let cachedTypedocSidebar: DefaultTheme.SidebarItem[] | undefined; | |
| function getTypedocSidebar() { | |
| if (cachedTypedocSidebar !== undefined) { | |
| return cachedTypedocSidebar; | |
| } | |
| const filepath = path.resolve( | |
| import.meta.dirname, | |
| '../reference/typedoc-sidebar.json', | |
| ); | |
| if (!existsSync(filepath)) { | |
| cachedTypedocSidebar = []; | |
| return cachedTypedocSidebar; | |
| } | |
| try { | |
| cachedTypedocSidebar = require(filepath) as DefaultTheme.SidebarItem[]; | |
| return cachedTypedocSidebar; | |
| } catch (error) { | |
| console.error('Failed to load typedoc sidebar:', error); | |
| cachedTypedocSidebar = []; | |
| return cachedTypedocSidebar; |
Signed-off-by: Kevin Deng <[email protected]>
No description provided.