chore(doctor): replace grunt with npm scripts and build to dist - #6093
Conversation
Sorry, something went wrong.
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.
|
Configuration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 1c2426d4-fae5-4f5e-becc-f3c83e0fa0d4 📥 CommitsReviewing files that changed from the base of the PR and between 1b5cabd and b7bdc72. 📒 Files selected for processing (17)
📝 Walkthrough WalkthroughThe doctor package replaces Grunt-based compilation and packaging with TypeScript and Node scripts, emits artifacts under dist, adds declaration contracts, updates cleanup and test fixture handling, and forwards Android runtime versions through Doctor validation. 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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify 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.
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
📥 CommitsReviewing files that changed from the base of the PR and between 1b5cabd and b7bdc72.
📒 Files selected for processing (17)- packages/doctor/.gitignore
- packages/doctor/.npmrc
- packages/doctor/Gruntfile.js
- packages/doctor/package.json
- packages/doctor/scripts/clean.js
- packages/doctor/scripts/copy-test-fixtures.js
- packages/doctor/src/android-tools-info.ts
- packages/doctor/src/doctor.ts
- packages/doctor/src/index.ts
- packages/doctor/test/sys-info.ts
- packages/doctor/test/wrappers/file-system.ts
- packages/doctor/tsconfig.json
- packages/doctor/tsconfig.release.json
- packages/doctor/tsconfig.test.json
- packages/doctor/tslint.json
- packages/doctor/typings/interfaces.d.ts
- packages/doctor/typings/nativescript-doctor.d.ts
- packages/doctor/.npmrc
- packages/doctor/tslint.json
- packages/doctor/Gruntfile.js
- packages/doctor/src/android-tools-info.ts
Sorry, something went wrong.
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 agentsVerify 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.
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
📥 CommitsReviewing files that changed from the base of the PR and between 1b5cabd and b7bdc72.
📒 Files selected for processing (17)- packages/doctor/.gitignore
- packages/doctor/.npmrc
- packages/doctor/Gruntfile.js
- packages/doctor/package.json
- packages/doctor/scripts/clean.js
- packages/doctor/scripts/copy-test-fixtures.js
- packages/doctor/src/android-tools-info.ts
- packages/doctor/src/doctor.ts
- packages/doctor/src/index.ts
- packages/doctor/test/sys-info.ts
- packages/doctor/test/wrappers/file-system.ts
- packages/doctor/tsconfig.json
- packages/doctor/tsconfig.release.json
- packages/doctor/tsconfig.test.json
- packages/doctor/tslint.json
- packages/doctor/typings/interfaces.d.ts
- packages/doctor/typings/nativescript-doctor.d.ts
- packages/doctor/.npmrc
- packages/doctor/tslint.json
- packages/doctor/Gruntfile.js
- packages/doctor/src/android-tools-info.ts
packages/doctor/typings/interfaces.d.ts (1)218-222: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expose getWarnings’ configuration parameter.
Doctor.getWarnings accepts and forwards config, but this public interface declares a zero-argument method. Typed consumers cannot request platform- or runtime-specific warnings.
Proposed fix📝 Committable suggestion- getWarnings(): Promise<IWarning[]>; + getWarnings( + config?: NativeScriptDoctor.ISysInfoConfig, + ): Promise<IWarning[]>;‼️ 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.🤖 Prompt for AI Agents/** * 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[]>;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.
Sorry, something went wrong.
|
On the second item from the review summary (typings/interfaces.d.ts, getWarnings not accepting the config parameter) — there is no inline thread for it, so replying here. Valid, and pre-existing. Doctor.getWarnings has taken config?: NativeScriptDoctor.ISysInfoConfig and forwarded it to getInfos since well before this PR, while the declaration says getWarnings(): Promise<IWarning[]> — so callers cannot pass the config without a cast. This PR only renames interfaces.ts to interfaces.d.ts; the contents are byte-identical. That rename is load-bearing rather than cosmetic: as a .ts it counted as a non-declaration input, which dragged tsc's inferred rootDir up to the package root and produced dist/src/... — breaking path.join(__dirname, "..", "resources", ...) in sys-info.ts, since dist/src/../resources is not <pkg>/resources. 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. |
Sorry, something went wrong.
PR Checklist
Companion to #6092, which does the same for the root package. packages/nativescript-envinfo already had this shape and is untouched.
What is the current behavior?
packages/doctor is still on Grunt, and npm install in it fails outright: grunt-tslint@5.0.2 peer-requires tslint@^5 against the tslint@6.1.3 devDependency. That was papered over in 2022 by committing a .npmrc containing legacy-peer-deps=true (356630a88).
tsc also emits next to the sources — main is src/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.
lint was load-bearing in name only: it currently fails with 6 errors, is wired into nothing (not build, test, prepack or 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 .npmrc goes too. The release workflow writes its own .npmrc with the auth token after npm pack, so it is unaffected.
istanbul dropped: 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.
types deliberately still points at the hand-written typings
types remains ./typings/nativescript-doctor.d.ts, and generated declarations do not replace it.
That file is not a normal module .d.ts — it is an ambient declare module "@nativescript/doctor" whose /// reference pulls in typings/interfaces.ts, declaring the global NativeScriptDoctor namespace. 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). Repointing types at dist/index.d.ts would silently delete the global and break the CLI's own typecheck.
Generated .d.ts are 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 .ts it emitted a pointless empty JS file — and, more importantly, counted as a non-declaration input, dragging tsc's inferred rootDir up to the package root and producing dist/src/.... That would have broken path.join(__dirname, "..", "resources", ...) in sys-info.ts, since dist/src/../resources is not <pkg>/resources.
Grunt was never applying the repo's compiler options
Worth knowing, because it explains the small source changes here. Gruntfile.js did options: grunt.file.readJSON("tsconfig.json").compilerOptions, and readJSON does not resolve extends. Grunt therefore passed only { skipLibCheck: true } and compiled with grunt-ts defaults, ignoring target, noImplicitAny, noUnusedLocals and isolatedModules from the root config.
Running real tsc surfaced pre-existing dead code, removed here:
Published 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 no engines, but it is a genuine change to the shipped artifact and worth a reviewer's attention.
Verification
Summary by CodeRabbit
New Features
Improvements
Bug Fixes