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

change properties for runTask to match ITaskIdentifier #155635

Closed
ivankravets opened this issue Jul 19, 2022 · 18 comments · Fixed by #156603
Closed

change properties for runTask to match ITaskIdentifier #155635

ivankravets opened this issue Jul 19, 2022 · 18 comments · Fixed by #156603
Assignees
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders tasks Task system issues
Milestone

Comments

@ivankravets
Copy link

ivankravets commented Jul 19, 2022

Does this issue occur when all extensions are disabled?: Yes

  • VS Code Version:
Version: 1.70.0-insider (Universal)
Commit: 92fd228156aafeb326b23f6604028d342152313b
Date: 2022-07-19T13:01:49.834Z
Electron: 18.3.5
Chromium: 100.0.4896.160
Node.js: 16.13.2
V8: 10.0.139.17-electron.0
OS: Darwin x64 21.5.0
  • OS Version: macOS

Steps to Reproduce:

  1. Register tasks
  2. Call with
    vscode.commands.executeCommand('workbench.action.tasks.runTask', {
      type: "PlatformIO",
      task: task.id,
    });

@yume-chan
Copy link
Contributor

refs #154854

@ArturoDent
Copy link

The format is args: { taskName?: string, type?: string}

from #155350

Are you asking to change the arg to task instead of taskName?

@igorskyflyer
Copy link

Please don't spread panic and use bold and capitalized words. Also, don't use hypothetical statistics. Regarding your issue, the properties of your arguments you passed to the function are wrong. The property is taskName, not task.

@ivankravets
Copy link
Author

@igorskyflyer, are you the VSCode dev? What was the goal of this issue?

@yume-chan
Copy link
Contributor

yume-chan commented Jul 20, 2022

Your code depends on an undocumented implementation detail of this command. Now it standardizes and properly exposes its arguments, which is different from the old format.

@ivankravets
Copy link
Author

Yes, this code has been working since 2017. Do you know in which version of VSCode API it was standardized?

@meganrogge
Copy link
Contributor

Supported args are string (the old way) | {taskName?: string, type?:string}

both of those work for me

@ivankravets
Copy link
Author

Version: 1.70.0-insider (Universal)
Commit: 3cbf3063281410d21951877eb628551809d70c6b
Date: 2022-07-25T05:27:27.755Z
Electron: 18.3.5
Chromium: 100.0.4896.160
Node.js: 16.13.2
V8: 10.0.139.17-electron.0
OS: Darwin x64 21.6.0

Didn't work for me. Works well with any VSCode version from 2016 to 1.69.2 (including). When did you rename task to the taskName? Does it mean that VSCode v1.70 will break PlatformIO?

@ivankravets
Copy link
Author

@meganrogge, where did you find that API? I've just checked the TaskDefinition:

    /**
     * A structure that defines a task kind in the system.
     * The value must be JSON-stringifyable.
     */
    export interface TaskDefinition {
        /**
         * The task definition describing the task provided by an extension.
         * Usually a task provider defines more properties to identify
         * a task. They need to be defined in the package.json of the
         * extension under the 'taskDefinitions' extension point. The npm
         * task definition for example looks like this
         * ```typescript
         * interface NpmTaskDefinition extends TaskDefinition {
         *     script: string;
         * }
         * ```
         *
         * Note that type identifier starting with a '$' are reserved for internal
         * usages and shouldn't be used by extensions.
         */
        readonly type: string;

        /**
         * Additional attributes of a concrete task definition.
         */
        [name: string]: any;
    }

The API has not been changed since the @dbaeumer defined it. See #29855

@meganrogge
Copy link
Contributor

This is not an API change. It's just arguments that can be provided to the command runTask

@meganrogge
Copy link
Contributor

Described here

private async _registerCommands(): Promise<void> {
CommandsRegistry.registerCommand({
id: 'workbench.action.tasks.runTask',
handler: async (accessor, arg) => {
if (await this._trust()) {
this._runTaskCommand(arg);
}
},
description: {
description: 'Run Task',
args: [{
name: 'args',
isOptional: true,
description: nls.localize('runTask.arg', "Filters the tasks shown in the quickpick"),
schema: {
anyOf: [
{
type: 'string',
description: nls.localize('runTask.label', "The task's label or a term to filter by")
},
{
type: 'object',
properties: {
type: {
type: 'string',
description: nls.localize('runTask.type', "The contributed task type")
},
taskName: {
type: 'string',
description: nls.localize('runTask.taskName', "The task's label or a term to filter by")
}
}
}
]
}
}]
}
});
CommandsRegistry.registerCommand('workbench.action.tasks.reRunTask', async (accessor, arg) => {
if (await this._trust()) {
this._reRunTaskCommand();
}
});

@ivankravets
Copy link
Author

@dbaeumer, what are your thoughts here? Now we have 2 interfaces for “task” entity/definition. What is the benefit to maintain 2 of them and make things complex? I even don’t say that existing extensions are broken with v1.70.

@ivankravets
Copy link
Author

ivankravets commented Jul 26, 2022

Supported args are string (the old way) | {taskName?: string, type?:string}

No, see the scheme for the latest stable 1.69 and "the old way":

https://github.com/microsoft/vscode/blob/1.69.0/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts#L348:L365

@meganrogge, @Tyriar, could you reopen this issue and implement it? Thanks in advance.

schema: {
  anyOf: [
    {
      type: 'string',
      description: nls.localize('runTask.label', "The task's label or a term to filter by")
    },
    {
      type: 'object',
      properties: {
        type: {
          type: 'string',
          description: nls.localize('runTask.type', "The contributed task type")
        },
        task: {
          type: 'string',
          description: nls.localize('runTask.taskName', "The task's label or a term to filter by")
        }
      }
    },
    {
      type: 'object',
      properties: {
        type: {
          type: 'string',
          description: nls.localize('runTask.type', "The contributed task type")
        },
        taskName: {
          type: 'string',
          description: nls.localize('runTask.taskName', "The task's label or a term to filter by")
        }
      }
    }
  ]
}

@meganrogge
Copy link
Contributor

are you requesting that taskName be changed to task? @ivankravets

the old schema did not have a task property either.

@ivankravets
Copy link
Author

Sorry, I don't want to disturb people anymore in this thread. We will refactor our codebase to the legacy form PlatformIO: ${task.name} and make the PlatformIO extension compatible with any VSCode versions. We don't know how it will affect the performance while searching for the matching tasks. We typically contribute 20-30 tasks depending on the user's project complexity.


But! Being on the API side, I don't like introducing a new task definition. If you want to rename "undocumented" task property, maybe, the better candidate is name? In this case, export interface TaskDefinition will be unified across VSCode's codebase.

the old schema did not have a task property either.

Yes, it was my typo, sorry. We got task-based definition many years ago from @dbaeumer. Maybe, that was the workaround to something else. I don't remember all details. It was 201 or 2018.

@meganrogge
Copy link
Contributor

@alexr00 do you have context for what Dirk did here or an opinion about this?

@alexr00
Copy link
Member

alexr00 commented Jul 27, 2022

It looks like the run task command used to support taking an ITaskIdentifier. The task argument mentioned in this issue doesn't need to be explicitly supported because it was implicitly supported through the ITaskIdentifier interface, which only requires a type property:

export interface ITaskIdentifier {
type: string;
[name: string]: any;
}

Now, the run task command doesn't support taking an ITaskIdentifier. While this wasn't defined as API, it was used as such. Given that this has been stable for years, my opinion is that we should continue to support taking an ITaskIdentifier.

Details about how ITaskIdentifier used to be supported:

any arg here gets passed to this._getTaskIdentifier():

private _runTaskCommand(arg?: any): void {
if (!this._canRunCommand()) {
return;
}
const identifier = this._getTaskIdentifier(arg);

_getTaskIdentifier checks if the any looks like an ITaskIdentifier then creates an identifier from it. This is where the task from the original issue comes from. It is a property of the ITaskIdentifier:

private _getTaskIdentifier(arg?: any): string | KeyedTaskIdentifier | undefined {
let result: string | KeyedTaskIdentifier | undefined = undefined;
if (Types.isString(arg)) {
result = arg;
} else if (arg && Types.isString((arg as ITaskIdentifier).type)) {
result = TaskDefinition.createTaskIdentifier(arg as ITaskIdentifier, console);
}
return result;
}

@meganrogge meganrogge changed the title URGENT: workbench.action.tasks.runTask is broken for >2,000,000 developers? change properties for runTask to match ITaskIdentifier Jul 27, 2022
@meganrogge meganrogge added debt Code quality issues tasks Task system issues labels Jul 27, 2022
@meganrogge meganrogge reopened this Jul 27, 2022
@meganrogge meganrogge added this to the July 2022 milestone Jul 28, 2022
meganrogge added a commit that referenced this issue Jul 28, 2022
meganrogge added a commit that referenced this issue Jul 28, 2022
@vscodenpa vscodenpa added the unreleased Patch has not yet been released in VS Code Insiders label Jul 28, 2022
@ivankravets
Copy link
Author

Thanks, @meganrogge, and @alexr00, for keeping compatibility with existing extensions 🙏

@vscodenpa vscodenpa added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Jul 29, 2022
meganrogge added a commit that referenced this issue Aug 11, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders tasks Task system issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants