Conversation
MilapNaik
changed the title
There was a problem hiding this comment.
Pull request overview
This PR carries forward recent CLI argument/behavior changes (from #202) into the underlying RokuDeploy.ts API and command implementations, aligning option names and expanding sideload() behavior.
Changes:
- Renames options across the API to match CLI (
remotePort→ecpPort,ZipOptions.stagingDir→dir,rekeySignedPackage→pkg). - Moves/centralizes path resolution logic into command classes and
RokuDeploy.sideload(). - Updates docs/changelog to reflect the new option names and
sideloadclose behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/commands/ZipCommand.ts | Adds cwd defaulting and resolves --out/--dir paths before calling zip(). |
| src/commands/StageCommand.ts | Maps --out to stagingDir for stage(). |
| src/commands/SideloadCommand.ts | Resolves zip/rootDir/outZip paths and forwards options to sideload(). |
| src/commands/RekeyDeviceCommand.ts | Adds cwd defaulting and resolves --pkg to an absolute path. |
| src/commands/CreateSignedPackageCommand.ts | Adds cwd defaulting and parses --out into outDir/outFile. |
| src/commands/CaptureScreenshotCommand.ts | Adds cwd defaulting and parses --out into screenshotDir/screenshotFile. |
| src/cli.ts | Removes inline argument post-processing; updates sideload flag to --close/--no-close; switches to ecpPort. |
| src/RokuDeployOptions.ts | Renames remotePort → ecpPort and rekeySignedPackage → pkg. |
| src/RokuDeploy.ts | Implements ZipOptions.dir; switches ECP usage to ecpPort; enhances sideload() (zip/rootDir support + channel close). |
| src/RokuDeploy.spec.ts | Updates tests for renamed options (pkg, dir, ecpPort). |
| README.md | Updates CLI/API docs for renamed options and new sideload behavior. |
| CHANGELOG.md | Adds 4.0.0-alpha.3 breaking-change notes covering the renames and sideload changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (args.out) { | ||
| args.out = path.resolve(args.cwd, args.out); | ||
| options.screenshotDir = path.dirname(args.out); | ||
| options.screenshotFile = path.basename(args.out); | ||
| } |
There was a problem hiding this comment.
CaptureScreenshotCommand sets options.screenshotFile to path.basename(args.out), which includes an extension (e.g. roku-deploy.jpg). RokuDeploy.captureScreenshot() appends the device-provided extension to screenshotFile, so this will create roku-deploy.jpg.jpg. Consider stripping the extension when setting screenshotFile (and treat --out as a full path prefix).
| // Resolve zip/rootDir before getOptions so outDir/outFile are set correctly | ||
| if (options.zip) { | ||
| options.zip = path.resolve(options.cwd ?? process.cwd(), options.zip); | ||
| options.outDir = path.dirname(options.zip); | ||
| options.outFile = path.basename(options.zip); | ||
| options.retainDeploymentArchive = true; | ||
| } else if (options.rootDir) { | ||
| options.rootDir = path.resolve(options.cwd ?? process.cwd(), options.rootDir); | ||
| } | ||
|
|
||
| options = this.getOptions(options) as any; | ||
|
|
||
| // Close the channel before sideloading unless explicitly disabled | ||
| if (options.close !== false) { | ||
| await this.closeChannel(options as CloseChannelOptions); | ||
| } | ||
|
|
||
| // If rootDir was provided (and no zip), zip it first then sideload | ||
| if (!options.zip && options.rootDir) { | ||
| await this.zip({ dir: options.rootDir, outDir: options.outDir, outFile: options.outFile, cwd: options.cwd }); | ||
| options.retainDeploymentArchive = false; | ||
| } |
There was a problem hiding this comment.
This code overwrites retainDeploymentArchive unconditionally (sets true when zip is provided, and later forces false when rootDir is provided). That prevents callers from honoring an explicit retainDeploymentArchive value. Only set this as a default when the caller didn’t provide a value (e.g. when retainDeploymentArchive is undefined).
| if (args.out) { | ||
| if (!args.out.endsWith('.pkg')) { | ||
| throw new Error('Out must end with a .pkg'); | ||
| } | ||
| args.out = path.resolve(args.cwd, args.out); | ||
| options.outDir = path.dirname(args.out); | ||
| options.outFile = path.basename(args.out); | ||
| } |
There was a problem hiding this comment.
options.outFile is being set to the full basename including the .pkg extension. getOutputPkgFilePath() always appends .pkg when outFile doesn’t end with .zip, so this will produce filenames like my-app.pkg.pkg. Strip the extension when setting outFile (or update getOutputPkgFilePath() to treat .pkg as already-suffixed).
| if (args.outZip) { | ||
| options.outZip = path.resolve(args.cwd, args.outZip); | ||
| } | ||
|
|
||
| if (args.ecpPort) { | ||
| options.remotePort = args.ecpPort; | ||
| } | ||
|
|
||
| if (args.noclose !== true) { | ||
| await rokuDeploy.closeChannel(options as CloseChannelOptions); | ||
| } | ||
|
|
||
|
|
||
| if (args.zip) { | ||
| options.retainDeploymentArchive = true; | ||
| await rokuDeploy.sideload(options); | ||
| } else if (args.rootDir) { | ||
| await rokuDeploy.zip(options); | ||
| options.retainDeploymentArchive = false; | ||
| await rokuDeploy.sideload(options); | ||
| } else { | ||
| throw new Error('Either zip or rootDir must be provided for sideload command'); | ||
| } | ||
| await rokuDeploy.sideload(options); |
There was a problem hiding this comment.
The CLI exposes --outZip for sideload, but this command only resolves it into options.outZip and RokuDeploy.sideload() never reads outZip (it uses outDir/outFile). As a result --outZip currently has no effect. Map outZip into outDir/outFile (like ZipCommand does) or remove the flag from the CLI/options.
| // Close the channel before sideloading unless explicitly disabled | ||
| if (options.close !== false) { | ||
| await this.closeChannel(options as CloseChannelOptions); | ||
| } | ||
|
|
||
| // If rootDir was provided (and no zip), zip it first then sideload | ||
| if (!options.zip && options.rootDir) { | ||
| await this.zip({ dir: options.rootDir, outDir: options.outDir, outFile: options.outFile, cwd: options.cwd }); | ||
| options.retainDeploymentArchive = false; |
There was a problem hiding this comment.
sideload() now closes the channel by default and can also auto-zip rootDir before uploading. The existing RokuDeploy.spec.ts sideload tests currently assume a fixed number/order of doPostRequest calls; they will need to be updated and new assertions added to cover (1) close-on-by-default, (2) close: false skipping the close, and (3) rootDir triggering a zip() call.
Extend CLI changes from #202 to Rokudeploy.ts. Done mostly by Claude Code.