refactor(node): extract @doc-kittens/node package - #961
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## refactor/legacy-kitten #961 +/- ##
==========================================================
- Coverage 84.54% 78.27% -6.27%
==========================================================
Files 215 215
Lines 19148 19151 +3
Branches 1670 1564 -106
==========================================================
- Hits 16188 14990 -1198
- Misses 2953 4153 +1200
- Partials 7 8 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview
Docs examples for the redesigned site use Reviewed by Cursor Bugbot for commit 59787df. Bugbot is set up for automated code reviews on this repo. Configure here. |
| return dedent` | ||
| 'use strict'; | ||
| const common = require('../../common'); | ||
| const common = require('@node-core/doc-kit/generators/common'); |
There was a problem hiding this comment.
Wrong require in generated tests
High Severity
updateJsRequirePaths now embeds require('@node-core/doc-kit/generators/common') into generated addon test.js files. Those files run under Node.js's test/addons tree and need the relative ../../common helper for common.buildType, so addon verification will fail to resolve the module.
Reviewed by Cursor Bugbot for commit 1043c7e. Configure here.
There was a problem hiding this comment.
🤦 Find + Replace always has quirks
|
why |
doc-kit is taken, and we can't claim it since we don't own a trademark for the term |
Is the goal to move away from @node-core scope to communicate its general availability? Cause I don't know if that's worth it. I don't recall if all of this was discussed earlier, or if consensus was reached. Apologies for missing it edit: #343 was the voted name. I understand the dynamics of the scope and ownership... edit: I'd prefer Is this open for revisiting. |
The goal is to put generators in their own scope to avoid cluttering the core scope. |
|
is there such a thing as cluttering? you cannot really see all packages in a scope - i can see them at https://npmx.dev/org/node-core but couldnt figure out an npmjs way to do this. adding a new scope increases our support footprint... what about |
https://www.npmjs.com/org/node-core
Sounds good to me, just need to name the packages something like |
1043c7e to
b654f0d
Compare
I wonder if people could mistake this as being @doc-kittens repackage FOR nodejs, instead of specific TO nodejs. I feel these should be actually under @node-core/doc-kittens or something @node-core as it is Node.js's specific and not for outer world usage, wdyt? |
There was a problem hiding this comment.
🟡 Not ready to approve
There are multiple concrete breakages/inconsistencies (missing @doc-kittens/node package manifest/exports, a runtime-invalid require() target, and an extractExports logic bug) that must be addressed before this can safely ship.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
packages/node/src/addon-verify/utils/generateFileList.mjs:12
@node-core/doc-kit/generators/commondoes not exist in this repo's@node-core/doc-kitpackage exports (there is nopackages/core/src/generators/common), so generatedtest.jsfiles will fail at runtime when required. Revert to the original relative Node.js test harness import (or introduce and export a real replacement module).
README.md:101- The redesigned docs example now uses
-t html, but the preceding sentence still says to use thewebgenerator; this is confusing given the newhtmltarget name (withwebbeing an alias). Update the text to match the command shown.
-t html \
.changeset/node-kitten-package.md:3
- This changeset marks
@node-core/doc-kitas a minor bump, but this PR removes previously exported subpaths like@node-core/doc-kit/man-page,@node-core/doc-kit/api-links, and@node-core/doc-kit/addon-verifyfrompackages/core/package.json, which is a breaking change for consumers. Either keep compatibility exports/re-exports, or bump@node-core/doc-kitto a major release.
'@doc-kittens/node': major
'@node-core/doc-kit': minor
---
- Files reviewed: 13/40 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| 'man-page': '@doc-kittens/node/man-page', | ||
| 'legacy-json': '@doc-kittens/legacy/legacy-json', | ||
| 'legacy-json-all': '@doc-kittens/legacy/legacy-json-all', | ||
| 'addon-verify': '@node-core/doc-kit/addon-verify', | ||
| 'api-links': '@node-core/doc-kit/api-links', | ||
| 'addon-verify': '@doc-kittens/node/addon-verify', | ||
| 'api-links': '@doc-kittens/node/api-links', |
b654f0d to
59787df
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 59787df. Configure here.
| 'www/out', | ||
| 'out/', | ||
| 'packages/core/src/generators/api-links/__tests__/fixtures/', | ||
| 'packages/node/src/api-links/__tests__/fixtures/', |
There was a problem hiding this comment.
Stale Prettier ignore path
Medium Severity
The ESLint ignore for api-links fixtures was updated to packages/node/..., but .prettierignore still points at packages/core/src/generators/api-links/__tests__/fixtures/. Those fixtures are intentionally non-Prettier-shaped, so format:check in CI can fail now that they are no longer ignored.
Reviewed by Cursor Bugbot for commit 59787df. Configure here.


And finally, the remaining Node.js-specific generators are now in
@doc-kittens/node