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 ? 78.43%
=========================================================
Files ? 214
Lines ? 19112
Branches ? 1564
=========================================================
Hits ? 14990
Misses ? 4114
Partials ? 8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview Moved code is updated to depend on Direct imports of Reviewed by Cursor Bugbot for commit 1043c7e. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1043c7e. 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
| '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', |
There was a problem hiding this comment.
Missing node package manifest
High Severity
The new @doc-kittens/node package is registered in the lockfile and wired up via shorthand aliases, but packages/node/package.json is absent. Without a manifest and exports map, imports such as @doc-kittens/node/man-page cannot resolve, so man-page, addon-verify, and api-links fail to load.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 1043c7e. 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 |
And finally, the remaining Node.js-specific generators are now in
@doc-kittens/node