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

Contributed Task Configuration question #4928

Closed
RomanNikitenko opened this issue Apr 17, 2019 · 15 comments
Closed

Contributed Task Configuration question #4928

RomanNikitenko opened this issue Apr 17, 2019 · 15 comments
Labels
question user / developer questions tasks issues related to the task system

Comments

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Apr 17, 2019

Description

My question is about ContributedTaskConfiguration.

As far as I understand, fields _source and _scope are required to have ability run task as 'provided' (in the new terms - 'detected') task.
At the same time field _scope can be undefined according to the doc.

For 'detected' tasks with _scope = undefined it works well expect one usecase.
After running task at least one time the task is moved to 'recently used tasks' section. Recently used tasks are saved to the storage when an application is stopped and the tasks are restored when an application is started.

The problem is: after starting an application tasks with _scope = undefined don't contain field _scope anymore, so are handled as 'configured'. As result we get error.

I'm looking for way how I can fix this case.
Is it possible to improve way of storage recently used tasks to do not lose fields with undefined value? (as we rely just on the presence of the field)
If not, should we have some default value instead of undefined?

Thanks in advance!

Reproduction Steps

  1. Run a detected task with _scope = undefined
  2. Stop an application
  3. Start an application
  4. Run the same task from 'recently used tasks' section

@elaihau @akosyakov

@RomanNikitenko RomanNikitenko added question user / developer questions tasks issues related to the task system labels Apr 17, 2019
@elaihau
Copy link
Contributor

elaihau commented Apr 17, 2019

Thank you for finding the bug and pointing out the root cause !

You are right. the undefined properties are removed when saved as json. Since null can be preserved, should we use null instead for those "global tasks" ? @RomanNikitenko @akosyakov

@akosyakov
Copy link
Member

@elaihau How about checking only presence of _source and mark _scope as optional?

@elaihau
Copy link
Contributor

elaihau commented Apr 24, 2019

I recently experimented adding a TaskDefinitionRegistry for the detected tasks, and am thinking maybe that can be used to determine the type of tasks (detected vs configured) instead of checking the presence of _scope: Simply speaking, any task that has a definition in the registry is a "detected task"

@RomanNikitenko
Copy link
Contributor Author

@elaihau
Did you apply your idea within #5024?
Is it expected that described problem will go away after merge the PR?

@elaihau
Copy link
Contributor

elaihau commented May 1, 2019

@elaihau
Did you apply your idea within #5024?
Is it expected that described problem will go away after merge the PR?

Yes and No.
TaskDefinitionRegistry is implemented in my PR, however, resolving the described problem requires the client code to register the task definition first.

Could you please give me an example of "detected tasks not having a scope"? Thank you !

@RomanNikitenko
Copy link
Contributor Author

@elaihau
We provide our tasks using plugin API.
Field scope is optional, so can be undefined.
We use own runner for our tasks, but it doesn’t matter - we get error earlier.

I just tested the described above problem for simple build task:
{"label":"build","source":"che","type":"che","command":"cd ${current.project.path} \n yarn","target":{},"previewUrl":"","$ident":5,"_source":"che"}

Variable ${current.project.path} is resolved by our resolver, but you can replace it by path to a project.
Fields $ident and _source was added at converting our task, here _sourceand _scope, for example.

@elaihau
Copy link
Contributor

elaihau commented May 13, 2019

@RomanNikitenko
in my PR, i removed ContributedTaskConfiguration.is() that's causing the problem:
https://github.com/theia-ide/theia/pull/5024/files#diff-b528ec8ff69f664cc264ee0eea5a1526L52

and used a different solution to check if a task comes from a Theia extension or plugin:
https://github.com/theia-ide/theia/pull/5024/files#diff-65aa48cf1be5fc5d2c202882762b24c4R330

The idea is that, if the Task is contributed / detected / provided by an extension or plugin, the task definition should have been registered and accessible from the TaskDefinitionRegistry. In other words, if TaskDefinitionRegistry.getDefinition(yourTask) returns you the definition, your task is from an extension / plugin.

However, this solution requires the developer to have the tasks defined either in theia plugin's package.json in this way:

    "taskDefinitions": [
      {
        "type": "grunt",
        "required": [
          "task"
        ],
        "properties": {
          "task": {
            "type": "string",
            "description": "%grunt.taskDefinition.type.description%"
          },
          "file": {
            "type": "string",
            "description": "%grunt.taskDefinition.file.description%"
          }
        }
      }
    ]

or register programmatically:

	        this.taskDefinitionRegistry.register({
            type: CPP_BUILD_TASK_TYPE_KEY,
            required: ['label'],
            properties: {
                label: {
                    type: 'string',
                    description: 'Lable of the cpp build configuration task'
                }
            }
        }, CPP_BUILD_TASK_SOURCE);

https://github.com/theia-ide/theia/pull/5024/files#diff-ae714101d957cb2926e97da3de5c6706R119

What do you think of it? Any suggestions ?

@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented May 15, 2019

@elaihau
Hello!
Looks fine for me!
So, all tasks are provided by plugin API should be automatically registered in taskDefinitionRegistry. Is it correct?

@elaihau
Copy link
Contributor

elaihau commented May 15, 2019

@RomanNikitenko
no, not automatically.
take vscode extension grunt for example, the developer of extension would have to add taskDefinition as part of contributes to have it registered: https://github.com/microsoft/vscode/blob/master/extensions/grunt/package.json#L46

a few other examples of contributing task definition(s) through package.json:

@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented May 16, 2019

@elaihau
So we have two ways to provide a 'detected' task:

Tasks form package.json will be registered automatically as I understand.

What we should do with second case?
Should we register tasks programmatically for second case?

@elaihau
Copy link
Contributor

elaihau commented May 16, 2019

in terms of we have two ways to provide a 'detected' task, my understanding is slightly different from yours.

in my opinion there is only one way to provide a 'detected' task, which is using the plugin API you referred to.

package.json is just the place to provide the definition of the detected task.

again, take the vscode grunt extension for example, it uses the plugin API to provide task objects: https://github.com/microsoft/vscode/blob/master/extensions/grunt/src/main.ts#L276

at the same time, it gives the definition of the task from package.json: https://github.com/microsoft/vscode/blob/master/extensions/grunt/package.json#L46

@RomanNikitenko @akosyakov please kindly let me know if i misunderstand the way it works, or miss anything. Thank you !

PS: @RomanNikitenko are you making the task contribution from a place where package.json is unavailable? I am curious what the use case is.

@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented May 16, 2019

@elaihau
About use case - we get our tasks using API and register them using Task provider.

thank you for your answer - I will investigate what we can do for our goals.

@elaihau
Copy link
Contributor

elaihau commented May 17, 2019

@RomanNikitenko i noticed that the constructor of theia.Task class takes a TaskDefinition argument. Maybe we could add some additional logic to theia, so that the definition registration can be done with data from theia.Task ?

@RomanNikitenko
Copy link
Contributor Author

@elaihau
I'm going to look into it this week.
thank you very much!

@RomanNikitenko
Copy link
Contributor Author

The issue is relevant for detected tasks, we consider che commands as configured theia tasks, so it’s not relevant for che commands anymore and I'm closing the issue.

@elaihau thank you for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question user / developer questions tasks issues related to the task system
Projects
None yet
Development

No branches or pull requests

3 participants