refactor(node): extract @node-core/doc-kit package, move core to @nodejs/doc-kit - #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 89.64% 89.63% -0.01%
==========================================================
Files 200 200
Lines 18495 18501 +6
Branches 1729 1729
==========================================================
+ Hits 16579 16584 +5
- Misses 1909 1910 +1
Partials 7 7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview
Legacy and React generator packages now depend on Reviewed by Cursor Bugbot for commit 6bf6bd3. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
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.
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.
59787df to
97cce4e
Compare
97cce4e to
15ba0be
Compare
bmuenzenmeyer
left a comment
There was a problem hiding this comment.
Approving the stack so we can test and iterate
15ba0be to
6bf6bd3
Compare

The remaining Node.js-specific generators (
api-links,addon-verify, andman-page) move intopackages/node, and the npm names are re-arranged to match the new layout:packages/core(engine + CLI)@nodejs/doc-kit(previously@node-core/doc-kit)packages/node(this PR)@node-core/doc-kit@node-core/doc-kitkeeps its npm name because these generators are specific to Node.js core, and the major bump signals the restructuring to existing consumers: v2 contains only the Node.js-specific generators, while the engine and CLI now live in@nodejs/doc-kit.