-
-
Notifications
You must be signed in to change notification settings - Fork 236
feat: Move javascript files to native typescript #2989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: jp/drop-node
Are you sure you want to change the base?
Conversation
|
15c05f5 to
a8e7f1f
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.
Bug: rejectOnError always rejects after resolving
When live is 'rejectOnError' and the command succeeds with exit code 0, the code resolves with 'success (live mode)' but then immediately continues to execute the reject statement because there's no return or else. This causes the promise to be settled twice and creates incorrect behavior where successful commands trigger rejection logic.
lib/helper.ts#L342-L348
Lines 342 to 348 in a8e7f1f
| pid.on('exit', (exitCode) => { | |
| if (live === 'rejectOnError') { | |
| if (exitCode === 0) { | |
| resolve('success (live mode)'); | |
| } | |
| reject(new Error(`Command ${args.join(' ')} failed with exit code ${exitCode}`)); | |
| } |
| * @param options More options to pass to the CLI | ||
| */ | ||
| constructor(configFile, options) { | ||
| constructor(public configFile: string | null, public options: SentryCliOptions) { |
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.
Bug: SentryCli constructor breaks backwards compatibility
The constructor declares both configFile and options as required parameters without default values, but existing code calls new SentryCli() with no arguments. The original JavaScript version allowed both parameters to be optional. This breaks backwards compatibility beyond just the import syntax change documented in the CHANGELOG.
|
|
||
| - Drop support for Node.js <18. The minimum required Node.js version is now 18.0.0 ([#2985](https://github.com/getsentry/sentry-cli/issues/2985)). | ||
| - The JavaScript wrapper now uses named exports instead of default exports ([#2989](https://github.com/getsentry/sentry-cli/pull/2989)) | ||
| . You need to update your imports: |
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.
a9f9165 to
a809a5c
Compare
a8e7f1f to
d5414fc
Compare
| * @param {OptionsSchema} [schema] An options schema required by the command. | ||
| * @param {object} [options] An options object according to the schema. | ||
| * @returns {string[]} An arguments array that can be passed via command line. | ||
| * @param command The literal name of the command. |
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.
Bug: Undefined paramName used in non-boolean types
When OptionsSchema entries have only invertedParam without param, the paramName variable becomes undefined. For non-boolean types like string or number, line 237 concatenates undefined into the arguments array, creating invalid command-line arguments. This case needs proper handling to skip entries without a param for non-boolean types.
| * @returns A promise that resolves when the release has been created. | ||
| */ | ||
| async new(release, options) { | ||
| async new(release: string, options: { projects?: string[] }): Promise<string> { |
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.
Bug: Required options parameter breaks backward compatibility
The new method requires the options parameter, but tests call it without arguments like releases.new('my-version'). The SentryCliReleases interface defines options as optional, and getProjectFlagsFromOptions accepts undefined with default parameter = {}. The parameter needs to be optional to match the interface and maintain backward compatibility.
(closes #2617)
This PR migrates all JavaScript files in the
lib/directory to native TypeScript, removing the need for JSDoc type annotations and improving type safety.Main Changes
.jsfiles to.tswith proper TypeScript syntaxexportsyntax instead ofmodule.exportsOptionsSchemais now a union to satisfySOURCEMAPS_OPTIONS, as this one didn't had the requiredparamlib/releases/optionshas changed its nameBreaking Changes
Module Exports
module.exports = SentryCli;(CommonJS)export class SentryCli { ... }(ES6 named export)We could have maintained backwards compatibility with
export = SentryCli;. The only problem were the types which needed to be exported as well. With ES6 named exports, we can now export all types alongside the class.Internal Improvements
Simplified
Releaseconstructornew Releases({ ...this.options, configFile })- configFile was merged into optionsnew Releases(this.options, configFile)- two separate parametersWith 2 parameters it's easier to maintain and makes the separation of concerns clearer.