chore(doctor): replace grunt with npm scripts and build to dist - #6093
Conversation
Grunt's tasks were thin wrappers around tsc, npm test and npm pack: - ts:devsrc -> tsc - ts:devall -> tsc -p tsconfig.test.json - ts:release_build -> tsc -p tsconfig.release.json - clean -> scripts/clean.js - watch:ts -> tsc --watch - shell:npm_test -> npm test - tslint:build -> dropped The lint task was already failing with 6 errors, was not wired into build, test, prepack or CI, and tslint itself is deprecated; .ts files here are covered by the repo-wide prettier lint-staged hook instead. Dropping grunt-tslint also resolves the ERESOLVE that made npm install in this package require --legacy-peer-deps, so the .npmrc pinning legacy-peer-deps=true goes away too. Compilation moves out of the source tree: src/ now builds to dist/ with declarations, and the test build goes to dist-test/. dist/ sits at the same depth as src/ did, so the __dirname/../resources lookup in sys-info is unaffected. package.json main follows the output; types keeps pointing at the hand-written typings/, which is the package's public contract and also supplies the global NativeScriptDoctor namespace the CLI depends on. typings/interfaces.ts becomes a .d.ts so that it stops emitting an empty JS file and stops dragging rootDir up to the package root. istanbul is dropped along with it: nothing consumed the coverage report, and it is unmaintained. Grunt read tsconfig.json with grunt.file.readJSON, which does not resolve "extends", so it compiled with tsc defaults rather than the repo's compiler options. Running tsc for real surfaces some dead code that noUnusedLocals rejects, removed here.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughThe doctor package replaces Grunt-based compilation and packaging with TypeScript and Node scripts, emits artifacts under ChangesDoctor package modernization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Npm
participant TypeScript
participant FixtureCopy
participant Mocha
participant PackageTarball
Npm->>TypeScript: compile source and tests
Npm->>FixtureCopy: copy non-TypeScript fixtures
FixtureCopy->>Mocha: provide dist-test/test
Mocha-->>Npm: return test results
Npm->>PackageTarball: package dist artifacts
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/doctor/test/sys-info.ts`:
- Around line 239-241: Update the spawnFromEvent mock in the
execute-correct-commands test to capture the received command and args while
still returning empty stdout and stderr. Retain or restore the assertion that
verifies the expected executable and arguments are passed.
In `@packages/doctor/typings/interfaces.d.ts`:
- Around line 218-222: Update the public getWarnings declaration in the relevant
Doctor interface to accept the configuration parameter that Doctor.getWarnings
already receives and forwards, while preserving its Promise<IWarning[]> return
type and existing warning behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c2426d4-fae5-4f5e-becc-f3c83e0fa0d4
📒 Files selected for processing (17)
packages/doctor/.gitignorepackages/doctor/.npmrcpackages/doctor/Gruntfile.jspackages/doctor/package.jsonpackages/doctor/scripts/clean.jspackages/doctor/scripts/copy-test-fixtures.jspackages/doctor/src/android-tools-info.tspackages/doctor/src/doctor.tspackages/doctor/src/index.tspackages/doctor/test/sys-info.tspackages/doctor/test/wrappers/file-system.tspackages/doctor/tsconfig.jsonpackages/doctor/tsconfig.release.jsonpackages/doctor/tsconfig.test.jsonpackages/doctor/tslint.jsonpackages/doctor/typings/interfaces.d.tspackages/doctor/typings/nativescript-doctor.d.ts
💤 Files with no reviewable changes (4)
- packages/doctor/.npmrc
- packages/doctor/tslint.json
- packages/doctor/Gruntfile.js
- packages/doctor/src/android-tools-info.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/doctor/test/sys-info.ts`:
- Around line 239-241: Update the spawnFromEvent mock in the
execute-correct-commands test to capture the received command and args while
still returning empty stdout and stderr. Retain or restore the assertion that
verifies the expected executable and arguments are passed.
In `@packages/doctor/typings/interfaces.d.ts`:
- Around line 218-222: Update the public getWarnings declaration in the relevant
Doctor interface to accept the configuration parameter that Doctor.getWarnings
already receives and forwards, while preserving its Promise<IWarning[]> return
type and existing warning behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c2426d4-fae5-4f5e-becc-f3c83e0fa0d4
📒 Files selected for processing (17)
packages/doctor/.gitignorepackages/doctor/.npmrcpackages/doctor/Gruntfile.jspackages/doctor/package.jsonpackages/doctor/scripts/clean.jspackages/doctor/scripts/copy-test-fixtures.jspackages/doctor/src/android-tools-info.tspackages/doctor/src/doctor.tspackages/doctor/src/index.tspackages/doctor/test/sys-info.tspackages/doctor/test/wrappers/file-system.tspackages/doctor/tsconfig.jsonpackages/doctor/tsconfig.release.jsonpackages/doctor/tsconfig.test.jsonpackages/doctor/tslint.jsonpackages/doctor/typings/interfaces.d.tspackages/doctor/typings/nativescript-doctor.d.ts
💤 Files with no reviewable changes (4)
- packages/doctor/.npmrc
- packages/doctor/tslint.json
- packages/doctor/Gruntfile.js
- packages/doctor/src/android-tools-info.ts
🛑 Comments failed to post (1)
packages/doctor/typings/interfaces.d.ts (1)
218-222: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expose
getWarnings’ configuration parameter.
Doctor.getWarningsaccepts and forwardsconfig, but this public interface declares a zero-argument method. Typed consumers cannot request platform- or runtime-specific warnings.Proposed fix
- getWarnings(): Promise<IWarning[]>; + getWarnings( + config?: NativeScriptDoctor.ISysInfoConfig, + ): Promise<IWarning[]>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Executes all checks for the current environment and returns the warnings from each check. * `@return` {Promise<IWarning[]>} Array of all the warnings from all checks. If there are no warnings will return empty array. */ getWarnings( config?: NativeScriptDoctor.ISysInfoConfig, ): Promise<IWarning[]>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/doctor/typings/interfaces.d.ts` around lines 218 - 222, Update the public getWarnings declaration in the relevant Doctor interface to accept the configuration parameter that Doctor.getWarnings already receives and forwards, while preserving its Promise<IWarning[]> return type and existing warning behavior.
|
On the second item from the review summary ( Valid, and pre-existing. This PR only renames Widening a published type signature is a public API change and does not belong in a build PR, so I am leaving it. Happy to open it as a follow-up — it is a one-line fix to make the declaration match the implementation. |
2 participants
PR Checklist
Companion to #6092, which does the same for the root package.
packages/nativescript-envinfoalready had this shape and is untouched.What is the current behavior?
packages/doctoris still on Grunt, andnpm installin it fails outright:grunt-tslint@5.0.2peer-requirestslint@^5against thetslint@6.1.3devDependency. That was papered over in 2022 by committing a.npmrccontaininglegacy-peer-deps=true(356630a88).tscalso emits next to the sources —mainissrc/index.js— so source and output share a directory, with the same consequences #6092 describes.What is the new behavior?
Grunt is gone; output goes to
dist/with declarations.ts:devsrc(default,build)tsc→dist/ts:devall(build.all)tsc -p tsconfig.test.json+scripts/copy-test-fixtures.js→dist-test/ts:release_buildtsc -p tsconfig.release.jsoncleanscripts/clean.jswatch:*tsc --watchpack(prepack)clean.build && npm test && tsc -p tsconfig.release.jsontslint:build(lint)lintwas load-bearing in name only: it currently fails with 6 errors, is wired into nothing (notbuild,test,prepackor CI), and tslint is deprecated — the repo lints via the root prettier/lint-staged hook. Dropping it is what removes the peer conflict, so the committed.npmrcgoes too. The release workflow writes its own.npmrcwith the auth token afternpm pack, so it is unaffected.istanbuldropped: coverage ran but nothing consumed it — no CI step, no coverage service config, no thresholds,coverage/gitignored.The release compile now runs after the tests rather than before, the same ordering fix #6088 made at the root.
typesdeliberately still points at the hand-written typingstypesremains./typings/nativescript-doctor.d.ts, and generated declarations do not replace it.That file is not a normal module
.d.ts— it is an ambientdeclare module "@nativescript/doctor"whose///reference pulls intypings/interfaces.ts, declaring the globalNativeScriptDoctornamespace. The CLI depends on that global in roughly 15 places with no import of the package at all (lib/declarations.d.ts,lib/common/doctor.d.ts,lib/common/declarations.d.ts,lib/services/doctor-service.ts,lib/sys-info.ts). Repointingtypesatdist/index.d.tswould silently delete the global and break the CLI's own typecheck.Generated
.d.tsare still emitted and shipped alongside, so deep imports get types they never had.One rename:
typings/interfaces.ts→typings/interfaces.d.ts. It contains only ambient declarations, so as a.tsit emitted a pointless empty JS file — and, more importantly, counted as a non-declaration input, dragging tsc's inferredrootDirup to the package root and producingdist/src/.... That would have brokenpath.join(__dirname, "..", "resources", ...)insys-info.ts, sincedist/src/../resourcesis not<pkg>/resources.Grunt was never applying the repo's compiler options
Worth knowing, because it explains the small source changes here.
Gruntfile.jsdidoptions: grunt.file.readJSON("tsconfig.json").compilerOptions, andreadJSONdoes not resolveextends. Grunt therefore passed only{ skipLibCheck: true }and compiled with grunt-ts defaults, ignoringtarget,noImplicitAny,noUnusedLocalsandisolatedModulesfrom the root config.Running real
tscsurfaced pre-existing dead code, removed here:src/android-tools-info.ts—getMaxSupportedCompileVersion, a private method with no callers anywhere in the reposrc/doctor.ts— an unusedprivate helpersconstructor parameter, plus its import and the argument at the single call site.Doctoris not reachable from the package's public exports, so this is not an API changetest/sys-info.ts—spawnFromEventCommand(written, never read) and an implicit-any[]return;test/wrappers/file-system.ts— an unusedrimrafimportPublished output moves from ES5 to ES2018 as a consequence of the tsconfig now actually applying. That matches the root package and
nativescript-envinfo, and the package declares noengines, but it is a genuine change to the shipped artifact and worth a reviewer's attention.Verification
npm installsucceeds with no.npmrcand no--legacy-peer-deps(521 → 197 packages)npm pack→ 32 files;main(dist/index.js),types(typings/nativescript-doctor.d.ts) and its referencedtypings/interfaces.d.ts, andresources/cocoapods-verification/cocoapods.zipall present; no.js.mapnode_modules(npm installthennpm pack) passes end to endcocoapods.zipresolves from the installeddist/, and live calls work (getNodeVersion(),getGitVersion(),doctor.getWarnings())NativeScriptDoctor.ISysInfoConfig/.ISysInfoData/.IInfo/.IWarningbareSummary by CodeRabbit
New Features
Improvements
Bug Fixes