v7 GA #621
v7 GA #621
Conversation
…titles, wired up prompting
| @@ -1,3 +1,69 @@ | |||
| 7.0.0 | |||
| ------------------- | |||
| * Require Node.js 18 or newer | |||
There was a problem hiding this comment.
I assume the CLI is only released with the next major release? Otherwise, some builds will fail after the package update, although the rest of the SDK is 16+
There was a problem hiding this comment.
v7 is slated for release Friday May 10, 2024. I've published 9 beta releases and 2 release candidates. I have resolved all issues. Nobody has reported a failed build using RC2. The CLI requires Node 18 because we don't have the resources to support Node 16. The next major release of the SDK can bump the required Node version to 18, but we don't have to until we update the dependencies in the SDK. This may not happen until 2 major releases from now, but by then we'll drop Node 18 too.
| - Removed `haxm` info | ||
| - Removed `sdk` setup | ||
| - Removed user locale setup | ||
| - Removed Xcode CLI tools detection |
There was a problem hiding this comment.
What is the replacement here?
There was a problem hiding this comment.
No. The SDK hasn't needed the Xcode CLI tools in years. It was dead code.
There was a problem hiding this comment.
Is there a reason why new files were created without TypeScript? It seems a bit oldskool :)
There was a problem hiding this comment.
Yeah, I'm not getting paid. 😉
There was a problem hiding this comment.
🫣 Nevermind the whole review then I guess.
| clean: 'removes previous build directories', | ||
| create: 'creates a new project', | ||
| project: 'get and set tiapp.xml settings' | ||
| }; |
There was a problem hiding this comment.
Custom indentation should be prevented
| */ | ||
| class GracefulShutdown extends Error {} | ||
|
|
||
| process.setMaxListeners(666); |
There was a problem hiding this comment.
Any reason for this amount of listeners?
There was a problem hiding this comment.
Node.js prints a warning if more than 10 listeners are added to an EventEmitter and the CLI plugin system uses an EventEmitter under the hood and 10 is not enough. Plus this line has been in the CLI for 11 years :) https://github.com/tidev/titanium-cli/blob/master/lib/titanium.js#L158
src/cli.js
Outdated
| * @access private | ||
| */ | ||
| applyArgv(cmd) { | ||
| if (Array.isArray(cmd?.options)) { |
src/cli.js
Outdated
| const argv = this.argv; | ||
| const cargv = cmd.opts(); | ||
| this.debugLogger.trace(`Copying ${cmd.options.length} options... (${cmd.name()})`); | ||
| for (const o of cmd.options) { |
| this.debugLogger.trace(`Creating ${hookType} hook "${name}"`); | ||
|
|
||
| return (...args) => { | ||
| let data = Object.assign(dataPayload || {}, { |
There was a problem hiding this comment.
Any reason why not to use ?? ?
There was a problem hiding this comment.
You're not wrong, but I've spent more time fixing bugs in this hook code than anywhere else. I don't want to touch this code if I don't have to.
| ({ | ||
| data, | ||
| platformInfo | ||
| } = await detect(cli.debugLogger, config, cli, types)); |
There was a problem hiding this comment.
This is how you destructure into existing variables.
src/commands/info.js
Outdated
| const types = {}; | ||
| let i = 0; | ||
| const specifiedTypes = (cli.argv.types || 'all').toLowerCase().split(','); | ||
| for (let t of specifiedTypes) { |
| async fn(logger, config, cli) { | ||
| const isJson = cli.argv.json || cli.argv.output === 'json'; | ||
| let projectDir; | ||
| let p = expand(cli.argv['project-dir'] || '.'); |
There was a problem hiding this comment.
In general: Check for nullish coalescing operators
There was a problem hiding this comment.
The || was intentional for those who pass in --project-dir "". || will handle empty strings while ?? will not.
src/util/detect.js
Outdated
| let name = process.platform; | ||
| let version = '?'; | ||
| let m; | ||
| let n; |
There was a problem hiding this comment.
You should see the original code. 🤣
I will fix the variable names here. Stay tuned!
There was a problem hiding this comment.
I think the changes are great and should be included asap. Some remarks that I found during a first quick review:
- New files should be written in TypeScript to make sure they can be better maintained long-term
- Abbreviations in property names should be omitted
- We have to make sure that this either gets released via a major SDK update (e.g. SDK 13) or remain Node-16 compatible in a "bridge" version. I would prefer the first choice.
This is a major release. See the changelog for details.