Skip to content
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

Migrating lib/api/client-commands/_base-command.js to typescript #3846

Closed

Conversation

itsspriyansh
Copy link
Contributor

I made changes to change the code from javascript to typescript but there are some typescript errors I am facing regarding which I need some guidance.

Line 61: Parameter '_' implicitly has an 'any' type.ts(7006)
Line 61: Parameter 'err' implicitly has an 'any' type.ts(7006)
Line 82: Property 'performAction' does not exist on type 'ClientCommand'.ts(2339)
Line 104, 105: Property 'originalTarget' does not exist on type 'string | Function'.
Property 'originalTarget' does not exist on type 'string'.ts(2339)
Line 115: Property 'transportActions' does not exist on type 'ClientCommand'.ts(2339)

@github-actions
Copy link

Status

  • ❌ No modified files found in the types directory.
    Please make sure to include types for any changes you have made. Thank you!.

userSuppliedCallback?: (param : any) => any;
fullResultObject?: boolean;
fullPromiseResolve?: boolean;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should create an interface for the parameters of a function only if we want to reuse it, otherwise just put the type beside each property in parameter:

static makePromise({
      performAction: (callback: (result: ResultObject) => void) => void,
      userSuppliedCallback: ((param : any) => any) = function() {}, 
      fullResultObject: boolean = true, 
      fullPromiseResolve: boolean = true
    }) : Promise<unknown> {}

@@ -76,11 +101,14 @@ module.exports = class ClientCommand {
* @param {function} callback
* @private
*/
executeScriptHandler(method, script, args, callback) {

transportActions : any = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mention all the properties available on this at the top of the class, along with public/private keywords.

if (script.originalTarget) {
script = script.originalTarget;
if ((script as any).originalTarget) {
script = (script as any).originalTarget;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to use any anywhere, unless absolutely necessary. So, we would need to make changes in the argument itself to accommodate originalTarget as well.

Ex. script: string | Function | {originalTarget: string | Function}

And then try to implement the DRY principal (string | Function are repeating twice above).

fullPromiseResolve?: boolean;
}

export default class ClientCommand {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be export = class ClientCommand, if it is being imported in other JS files, so that we don't need to make any changes in those JS files. (export = is similar to module.exports = in JS)

^ is the reason why the tests are failing.

And when converting those JS files (in which ClientCommand is being used) to TS, we will use import ClientCommand = require('./_base-command.js'); instead of import ClientCommand from './_base-command.js';.

Once we are sure that we are importing this file in TS files only, we can change the export back to export default class ClientCommand, and the imports in other TS files to import ClientCommand from './_base-command.js';.

@gravityvi gravityvi marked this pull request as draft August 25, 2023 08:08
@beatfactor beatfactor closed this Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants