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

Add "identifier" to object in vscode.tasks api #57707

Closed
ejizba opened this issue Aug 31, 2018 · 15 comments
Closed

Add "identifier" to object in vscode.tasks api #57707

ejizba opened this issue Aug 31, 2018 · 15 comments
Assignees
Labels
info-needed Issue requires more information from poster tasks Task system issues

Comments

@ejizba
Copy link

ejizba commented Aug 31, 2018

I'm calling the following api:

const tasks: vscode.Task[] = await vscode.tasks.fetchTasks();

Here is how my task is defined:

{
  "version": "2.0.0",
  "tasks": [
    {
      "label": "Example Task",
      "identifier": "exampleTaskId",
      "command": "run some stuff",
      "type": "shell",
      "presentation": {
        "reveal": "always"
      }
    }
  ]
}

But I can't seem to find the 'identifier' in the Task object returned by that api. I see the 'name' property, which seems to correspond to the 'label' - and I see a generated guid for definition.id

screen shot 2018-08-31 at 11 25 11 am

I don't want to query based on the label because that could change based on a user's language or whim. I would much prefer to query based on an id. Can we get "identifier" added to the Task object?

@vscodebot vscodebot bot added the tasks Task system issues label Aug 31, 2018
@dbaeumer
Copy link
Member

dbaeumer commented Sep 3, 2018

Using the identifier is not save either since a user can change it. This is why I didn't expose it in the first place. It is usually an identifier used for dependsOn clauses.

What exactly do you want to achieve

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Sep 3, 2018
@ejizba
Copy link
Author

ejizba commented Sep 4, 2018

Here's our most common scenario where we want this:

  1. User creates a C# project for Azure Functions
  2. User starts deploying that project through our extension
  3. We check the value of the user's azureFunctions.preDeployTask setting and see that it is publish
  4. We query for the publish task, run a dotnet publish before deploying, and deploy the result of that

I work on several Azure extensions and most of them support some sort of "deploy". In some cases we have templates so we can automatically create the task and settings to run a preDeployTask, but in other cases we expect users to manually add this to their projects if desired.

I'm not trying to find something that's "safe" because of course the user can do anything to their tasks.json and there isn't a completely "safe" option. I'm just trying to find the safest option and I think that's querying for a task based on identifier instead of label. We will display an error to the user if we can't find the task we're looking for and I think that's fine. After all, why does dependsOn use identifier instead of label? Users could break that, too, but it's still the best option.

It honestly just feels like an established pattern that you should query based on id instead of label. I expect our users know that and are less likely to change their identifier. Plus it's not just that I think labels could change - I think labels should change. Labels are displayed in the UI and thus should be translated into a user's language. Identifiers are never displayed in the UI.

@dbaeumer
Copy link
Member

dbaeumer commented Sep 4, 2018

But 99% of the tasks don't have an identifier and the task system doesn't advocate to define one. It is only there if there are two user defined tasks in the system that have the same label to make the unique for the dependsOn clause.

In the C# case the task is I assume contributed by the C# extension using the provider API. In this case the task has an identifer (e.g. the TaskIdentifier) for which API exists. Why can't you use that one?

And for the dependsOn we now even allow to put an extension defined task identifier. So if the system has an npm task named 'install' the the user can reference this task in the dependsOn property using the literal { type: 'npm', script: 'install' }. Such tasks will very likely never receive an 'identifier' property since they never need one to be referenced. Even not in a launch.json

@dbaeumer
Copy link
Member

dbaeumer commented Sep 4, 2018

If publish tasks become important and providers of a task want to mark them independent of the name as a publish task we should add another TaskGroup named publish

@ejizba
Copy link
Author

ejizba commented Sep 4, 2018

"the task system doesn't advocate to define one" where is this documentation? And why is this the case?

Our extension controls the entire flow from 'Create new Project', 'F5', to 'Deploy'. So no, this task is not contributed by the C# extension. We put down a new tasks.json file for our users when they create the project from our extension and we've been defining the identifier for them. publish was just one example. We also have a installFuncExtensions task which uses the Azure Functions cli to run func extensions install before publishing a JavaScript project. Whatever support the dotnet cli has, I guarantee there's no support for the Functions cli in VS Code (unless we added it)

@dbaeumer
Copy link
Member

dbaeumer commented Sep 4, 2018

"the task system doesn't advocate to define one" where is this documentation? And why is this the case?

It is not a mandatory attribute so users only add it if necessary.

But if you put down the tasks.json file why don't you contribute the tasks via the API then. Writing a tasks.json file is highly discouraged and was only something we ask extensions to do to work around the fact that there was no API to provide tasks. If there is a seconds extension that puts down a tasks.json file your tasks might simply be overridden.

@ejizba
Copy link
Author

ejizba commented Sep 4, 2018

It is not a mandatory attribute so users only add it if necessary.

I still don't see the problem here. We will likely check the identifier first and the label second if the identifier is not defined. Isn't that how "dependsOn" does it? There are other cases like VS Code's TreeItem interface here which has an optional id attribute that is based on the label if the id isn't defined.

We're not providing tasks to existing projects - we're creating an entirely new project (including a csproj file, cs files, gitignore, launch.json, tasks.json, and other files specific to Azure Functions). We never overwrite or edit an existing tasks.json file. If another extension overwrites the user's tasks.json, then that's a bug in their extension.

That being said, I'm open to moving to "contribute the tasks via the API then", but that feels separate than the current discussion (we've had our tasks like this for over a year now with no real issues). More importantly, our users could create any task (i.e. an arbitrary powershell script) that they want to run before deploying. If they define an "identifier" (which shows up in IntelliSense when you're editing a tasks.json file, so I think it's pretty likely some of our users will) I still don't get why we shouldn't respect that?

@dbaeumer
Copy link
Member

dbaeumer commented Sep 5, 2018

The problem is that you ask me to make something API that only exist due to legacy. Making it API results in the fact that we need to maintain the API and even maintain the feature.

To clarify things:

  • tasks do have an identifier (e.g. TaskDefinition https://github.com/Microsoft/vscode/blob/master/src/vs/vscode.d.ts#L4693) when contributed via the API (which is the preferred way over writing to a tasks.json file). This allows to uniquely identify a task.
  • if a task needs to be grouped there is the concept of a TaskGroup. Currently the API supports build, test, clean, rebuild. If we think that publish should be a grouping / tagging as well we should add it to the TaskGroup. Or make the task group expandable so that extensions can define their own groups / tags. This will allow users to tag tasks as publish tasks even if these tasks are defined in the tasks.json.

Instead of exposing a legacy feature I think your extension should use the new supported feature and we should improve those instead.

Any objection to close the issue.

@dbaeumer
Copy link
Member

dbaeumer commented Sep 5, 2018

I created #57946 to make clear that identifier is a deprecated concept.

@ejizba
Copy link
Author

ejizba commented Sep 5, 2018

The interface you linked to only has "type" and "additional attributes". What part of that is the identifier? Do we have to make our own id property in the "additional attributes" part?

The "legacy feature" that we've been using is just custom tasks. "identifier" serves a definite and valuable purpose in custom tasks. Are you saying custom tasks are deprecated?

@dbaeumer
Copy link
Member

dbaeumer commented Sep 5, 2018

No, the identifier in custom tasks is deprecated. I was introduce to allow users to make auto detected tasks referencable in dependsOn and launch.json. User defined task can always be referenced by the label since the user defines it in the tasks.json anyways.

So custom tasks are not deprecated. What is deprecated

  • id for custom tasks since it was only useful for legacy auto detected tasks
  • writing into the tasks.json to contribute tasks from an extension. This should now be done using a task provider.

Every extension can define its own TaskDefinition schema in the package.json using the taskDefinitions contribution point. See https://code.visualstudio.com/docs/extensions/example-tasks & https://code.visualstudio.com/docs/extensionAPI/extension-points#_contributestaskdefinitions for documentation.

@ejizba
Copy link
Author

ejizba commented Sep 5, 2018

Okay I think this can be closed if identifier is deprecated. Although I'm confused by the message you added:

Use task identifier literals instead when referencing a task in a dependsOn property or in a launch.json file.

Shouldn't you just say to use the task's label? "task identifier literal" confuses me and sounds too similar to the actual name of the property itself (identifier)

Thanks for bearing with me and I look forward to trying out the recommended contribution points

@dbaeumer
Copy link
Member

dbaeumer commented Sep 6, 2018

Thanks for pointing this out. Actually there are two different solutions. Either the name for custom tasks and the task definition for contributed tasks. I will adopt the message.

@dbaeumer
Copy link
Member

dbaeumer commented Sep 6, 2018

Closing the issue.

@dbaeumer dbaeumer closed this as completed Sep 6, 2018
@dbaeumer
Copy link
Member

dbaeumer commented Sep 6, 2018

Ping me if something is unclear with the task provider.

@vscodebot vscodebot bot locked and limited conversation to collaborators Oct 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
info-needed Issue requires more information from poster tasks Task system issues
Projects
None yet
Development

No branches or pull requests

2 participants