build: emit to dist/ and ship generated declarations - #6092
Merged
NathanWalker merged 1 commit intoJul 28, 2026
Merged
Conversation
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
Copy link
Copy Markdown
WalkthroughThe package now builds and assembles publishable files under dist, runs tests against compiled output, and creates archives with npm pack ./dist. Cleanup, GA ID tooling, packaging guards, and backup constructor typing were also updated. 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)
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.
5 tasks
Copy link
Copy Markdown
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@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.
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
📥 CommitsReviewing files that changed from the base of the PR and between 1b5cabd and 25265f1.
📒 Files selected for processing (11)- .github/workflows/npm_release_cli.yml
- .gitignore
- lib/services/project-backup-service.ts
- package.json
- scripts/clean.js
- scripts/copy-assets.js
- scripts/guard-root-pack.js
- scripts/set-ga-id.js
- test/.mocharc.yml
- tsconfig.json
- tsconfig.release.json
Sorry, something went wrong.
Comment thread
package.json
Outdated
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.
edusperoni
force-pushed
the
chore/build-to-dist
branch
from
25265f1 to
ea24c4d
Compare
July 28, 2026 20:47
Copy link
Copy Markdown
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@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.
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
📥 CommitsReviewing files that changed from the base of the PR and between 25265f1 and ea24c4d.
📒 Files selected for processing (11)- .github/workflows/npm_release_cli.yml
- .gitignore
- lib/services/project-backup-service.ts
- package.json
- scripts/clean.js
- scripts/copy-assets.js
- scripts/guard-root-pack.js
- scripts/set-ga-id.js
- test/.mocharc.yml
- tsconfig.json
- tsconfig.release.json
- .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.
Comment thread
scripts/set-ga-id.js
Comment on lines
+15
to
+17
| const dirIndex = process.argv.indexOf("--dir"); | ||
| const baseDir = | ||
| dirIndex === -1 ? rootDir : path.resolve(rootDir, process.argv[dirIndex + 1]); |
Copy link
Copy Markdown
There was a problem hiding this comment.
🎯 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);
}
JSRepository: 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);‼️ 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.
Suggested change
| 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); |
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.
NathanWalker
approved these changes
Jul 28, 2026
This was referenced Jul 28, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Checklist
What is the current behavior?
tsc emits next to each source file, so lib/ holds both the TypeScript and its output. That has cost us repeatedly:
There is also no coherent published type surface: 60 hand-written .d.ts ship, but the package declares no types entry, so consumers get nothing.
What is the new behavior?
Build output goes to dist/, and dist/ is assembled as a complete package root rather than just compiled JS.
That last part is the crux. lib/ resolves its siblings through __dirname — ../package.json, ../docs/helpers, ../../config, ../../vendor/gradle-plugin, ../../../vendor/aab-tool/bundletool.jar. Mirroring resources/, docs/, config/, vendor/, bin/ and setup/ alongside lib/ means all 46 of those paths keep working with no code changes, and the published tarball keeps exactly the internal layout it has today. Only where it is built from changed.
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:
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:
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
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