GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — build: emit to dist/ and ship generated declarations by edusperoni · Pull Request #6092 · NativeScript/nativescript-cli Conversation
|
tsconfig.json |
gains rootDir/outDir (rootDir is required, or tsc flattens the tree) |
tsconfig.release.json |
builds lib/ only, with declaration: true — this is what gets packed |
scripts/copy-assets.js |
mirrors assets, writes dist/package.json |
| packing | npm run pack → npm pack ./dist; prepack/postpack are gone |
392 generated declarations now ship, replacing the hand-written set as the emitted API surface. Adding a types entry is a deliberate follow-up — worth doing on its own merits rather than smuggling a new public contract into a build change.
Two behaviours improve as a side effect:
- The GA id no longer touches your working tree. It is set inside
dist(set-ga-id.js live --dir dist), so a failed pack can no longer leave a checkout configured to report as production. That is the concern raised on chore: replace grunt with npm scripts #6088, resolved by the design rather than bolted on. - A guard refuses to pack the repository root, which would otherwise nest everything one level deeper and silently break every
__dirnamepath. Verified it blocks barenpm packand does not fire fornpm pack ./dist.
Two bugs this caught while being built
Worth recording, because both produce a successful build with a subtly wrong artifact and neither is visible to the test suite:
- The asset copy initially clobbered fresh output.
lib/still contained 375 stale compiled.jsfrom the old in-place build (against 7 genuine source.js— vendored scripts, hooks, fixtures). The copy step now treats a.jswith a sibling.tsas compiler output and skips it. README.md,LICENSEandCHANGELOG.mddropped out of the tarball. npm takes those from the directory being packed, which is nowdist. Caught by diffing the packed file list against the published9.1.0-alpha.15.
Hand-written .d.ts are also copied through: tsc treats them as inputs and never emits them, and 115 generated declarations import from them — leaving them behind shipped types with dangling references.
Verification
1513 passing, 9 pending— unchanged, now running fromdist/- Packed file list vs published
9.1.0-alpha.15: the only absences arelib/.d.ts(the grunt-ts aggregator, removed in chore: replace grunt with npm scripts #6088) and the unused cross-client enum (removed in refactor: move ambient const enums into real modules #6089) — both already gone frommain - Tarball installed into a clean consumer: CLI runs,
--versionreports, help renders 56 rows across 6 sections, sibling paths resolve, sample generated declaration has zero dangling imports - Shipped
config.jsoncarries the live GA id while the working tree keeps dev npm run clean.buildleaves zero compiler output inlib//test/
Note for reviewers
.github/workflows/npm_release_cli.yml changes from npm pack to npm run pack. Without that the release would produce a tarball with everything nested under dist/. The publish job is unaffected — it publishes a tarball, so no lifecycle scripts run.
Summary by CodeRabbit
- Build & Packaging
- Improved package creation for more reliable CLI distribution.
- Packages now publish only the built
dist/output and point the CLI entry to the built artifact. - Added safeguards to prevent packing from the repository root.
- Improved asset copying for consistent runtime distribution.
- Testing
- Tests now run against the built distribution output (
dist/).
- Tests now run against the built distribution output (
- Maintenance
- Improved cleanup of generated build files.
- Added an option to select a configuration directory when setting or verifying analytics IDs.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
coderabbitai
Bot
commented
Jul 28, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
📝 WalkthroughWalkthroughThe package now builds and assembles publishable files under ChangesDistribution packaging flow
Backup constructor typing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant npmScripts
participant copy-assets.js
participant dist
participant npmPack
ReleaseWorkflow->>npmScripts: Run npm run pack
npmScripts->>copy-assets.js: Build release assets
copy-assets.js->>dist: Assemble publishable package
npmScripts->>npmPack: Pack ./dist
npmPack->>ReleaseWorkflow: Upload generated archive
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 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 `@package.json`:
- Around line 19-23: Update the package scripts so the normal build invoked by
the test script removes stale dist output before compiling. Add or reuse a
clean.build step and invoke it at the start of build, preserving the existing
tsc, dependency-generation, and asset-copy steps.
🪄 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: ce285316-5cd6-4a7f-830a-f17eabd66c80
📒 Files selected for processing (11)
.github/workflows/npm_release_cli.yml.gitignorelib/services/project-backup-service.tspackage.jsonscripts/clean.jsscripts/copy-assets.jsscripts/guard-root-pack.jsscripts/set-ga-id.jstest/.mocharc.ymltsconfig.jsontsconfig.release.json
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Compiling next to the sources has cost us repeatedly: a deleted spec left its .js behind and kept running for ten months, prepack silently undid its own release compile because both wrote to the same place, and .gitignore needs a blanket *.js plus a growing list of negations to tell source from output apart. dist/ is assembled as a complete package root rather than just compiled output. lib/ resolves its siblings through __dirname - ../package.json, ../docs/helpers, ../../config, ../../vendor/gradle-plugin - so resources, docs, config, vendor, bin and setup are mirrored alongside it and all 46 of those paths keep working untouched. The published tarball has the same internal layout as before; only where it is built from changed. - tsconfig gains rootDir/outDir; tsconfig.release.json builds lib only, with declarations, and is what gets packed - scripts/copy-assets.js mirrors assets and writes dist/package.json - packing is npm run pack (npm pack ./dist); prepack/postpack are gone and a guard refuses to pack the root, which would nest everything a level deeper - the GA id is now set inside dist, so a failed pack can no longer leave a checkout configured to report as production A .js with a sibling .ts is compiler output, so the copy step skips it - without that it would overwrite what tsc just emitted with whatever the old in-place build left behind. Hand-written .d.ts are copied too: tsc treats them as inputs and never emits them, and 115 generated declarations import from them. ProjectBackupService.Backup is annotated because declaration emit cannot describe an anonymous class expression that has private members. 1513 passing, unchanged. Verified by packing and installing the tarball into a clean consumer: the CLI runs, help renders, the sibling paths resolve, and the shipped config carries the live GA id while the working tree keeps dev.
25265f1 to
ea24c4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 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 `@scripts/set-ga-id.js`:
- Around line 15-17: Update the `baseDir` calculation around `dirIndex` to
validate the argument after `--dir` before passing it to `path.resolve`; accept
only a non-empty directory value that is not another `--*` option, otherwise
preserve the usage/error path for invalid input. Use the validated target value
when resolving the directory.
🪄 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: b288ce0b-f9f6-4291-a290-da0d95d15b25
📒 Files selected for processing (11)
.github/workflows/npm_release_cli.yml.gitignorelib/services/project-backup-service.tspackage.jsonscripts/clean.jsscripts/copy-assets.jsscripts/guard-root-pack.jsscripts/set-ga-id.jstest/.mocharc.ymltsconfig.jsontsconfig.release.json
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/npm_release_cli.yml
- tsconfig.json
- .gitignore
- test/.mocharc.yml
- lib/services/project-backup-service.ts
- package.json
- scripts/guard-root-pack.js
- tsconfig.release.json
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| const dirIndex = process.argv.indexOf("--dir"); | ||
| const baseDir = | ||
| dirIndex === -1 ? rootDir : path.resolve(rootDir, process.argv[dirIndex + 1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Find file" fd -a 'set-ga-id\.js$' . || true echo "File contents" if [ -f scripts/set-ga-id.js ]; then cat -n scripts/set-ga-id.js fi echo "Path.resolve undefined probe" node - <<'JS' const path = require('path'); try { console.log(path.resolve('/root', undefined)); } catch (e) { console.log("throws", e.name + ": " + e.message); } JS
Repository: NativeScript/nativescript-cli
Length of output: 1829
Validate the --dir value before resolving it.
node scripts/set-ga-id.js live --dir passes undefined to path.resolve, throwing before the usage branch handles invalid input. Gate --dir on a non-empty string or --* option and use that target value in path.resolve.
Proposed fix
const dirIndex = process.argv.indexOf("--dir");
+const targetDir = dirIndex === -1 ? undefined : process.argv[dirIndex + 1];
+if (dirIndex !== -1 && (!targetDir || targetDir.startsWith("--"))) {
+ console.error(
+ "Usage: node scripts/set-ga-id.js <live|dev|verify> [--dir <path>]"
+ );
+ process.exit(1);
+}
const baseDir =
- dirIndex === -1 ? rootDir : path.resolve(rootDir, process.argv[dirIndex + 1]);
+ targetDir === undefined ? rootDir : path.resolve(rootDir, targetDir);📝 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.
| const dirIndex = process.argv.indexOf("--dir"); | |
| const baseDir = | |
| dirIndex === -1 ? rootDir : path.resolve(rootDir, process.argv[dirIndex + 1]); | |
| const dirIndex = process.argv.indexOf("--dir"); | |
| const targetDir = dirIndex === -1 ? undefined : process.argv[dirIndex + 1]; | |
| if (dirIndex !== -1 && (!targetDir || targetDir.startsWith("--"))) { | |
| console.error( | |
| "Usage: node scripts/set-ga-id.js <live|dev|verify> [--dir <path>]" | |
| ); | |
| process.exit(1); | |
| } | |
| const baseDir = | |
| targetDir === undefined ? rootDir : path.resolve(rootDir, targetDir); |
🤖 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 `@scripts/set-ga-id.js` around lines 15 - 17, Update the `baseDir` calculation
around `dirIndex` to validate the argument after `--dir` before passing it to
`path.resolve`; accept only a non-empty directory value that is not another
`--*` option, otherwise preserve the usage/error path for invalid input. Use the
validated target value when resolving the directory.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.