Vscode: Allow extensions to set configuration options.

[mitchdenny]

I've got an extension that I'm currently adding telemetry too so that I can better understand how people are using it, however I want to give users an easy way to opt out of this telemetry collection. My initial thoughts were that I would display a quick pick with two options (agree or disagree) and that I would then store their choice for later executions.

However, it looks like the configuration API for extensions doesn't support the option to set a configuration value. It would be great if I could do something like this:

let configuration = vscode.workspace.getConfiguration('ecdc');
configuration.set('collectTelemetry', false);

I'm sure other extensions would find this useful as well to help guide users to setting up sensitive defaults.

[jrieken]

@mitchdenny You can also use the extension state for storage (https://code.visualstudio.com/docs/extensionAPI/vscode-api#ExtensionContext)

[mitchdenny]

Thanks @jrieken. I'll look into that in the meantime. Are you considering allowing extensions to write settings or is using extension state the better approach for this kind of setting?

[jrieken]

Not decided yet. The model of today is that only the user can modify settings and there is lot of good about that, but also see some shortcomings... The extension state is meant for remembering a user choice, store some installing path etc. This also includes your use-case. Since it's a little touchy (collecting telemetry data) I can understand that you want a more prominent option to unset something like that.

[mitchdenny]

Thanks for getting back to me. Appreciate it.

[jrieken]

Closing as we believe the current story isn't too bad. User manually editor configuration options and extensions can persist settings etc globally/locally

[jrieken]

We have decided to open up that way - giving extensions the power to modify the configuration outweighs the risks.

[jrieken]

@egamma, @aeschli - I wonder if I should implement a restriction wrt setting configurations. For me it makes sense that you cannot write a setting which isn't defined anywhere, like "editor.foobar": true will throw unless _some_ extension has contributed this using the contribution-point. What do you think?

[aeschli]

Given that our settings are just key-value pairs, why would we care about settings like 'foo.bar'.
We also have some hidden settings that aren't defined in the settings and there's always the the case that an extension is not present.
I'd rather worry about validating the type of the setting (string, number, boolean...)

[jrieken]

Given that our settings are just key-value pairs, why would we care about settings like 'foo.bar'.

Because they end up in a file which is supposed to be human readable, if an extension by accident fills it up with random stuff your configuration is effectively lost.

[jrieken]

There is a challenge with this. Generally, our configuration is flat key-value pairs. The catch is that keys can be dot-separated and that the configuration is an object-tree along those dots.

For instance, the setting search.exclude is a nested structure of JS objects { search: { exclude: ... } }. That brings in an ambiguity when the value part is also an object, like { search: { exclude: { '**/node_modules': true, '**/bar': true } } }.

For reading, we don't differentiate this. You can do getConfiguration('search').get('exclude.**/node_modules') === true or getConfiguration('search.exclude').get('**/node_modules') === true which I think is fair for reading but I am unsure how writing configurations should behave in such cases? Should we allow for partial changes of the value part when the key is actual key + value-key?

@bpasero @aeschli ideas?

[bpasero]

Btw I would find it a good thing if an extension could actually write to extensions it does not contribute because this opens the door for a whole new family of extensions that could provide added value for changing settings (e.g. toggle core settings via command palette or ask the user if he is a tabs user or not and then make all the settings changes to disable or enable tabs).

As for writing settings, not sure. I think the main use case is to write a value for a setting that shows up in user settings. If there is search.exclude you should be able to write to this value using search.exclude and not via search or search.exclude.when. There are (unfortunately) some keys that have 2 dots (e.g. html.format.indentInnerHTML) and again I would expect to write html.format.indentInnerHTML via the API to provide a value.

Now, I am not sure if you could enforce this at all given that in the end all of these things end up being one large object.

Unrelated: would this API take into consideration that there are global settings and workspace settings?

[jrieken]

Btw I would find it a good thing if an extension could actually write to extensions it does not contribute because

There is a misunderstanding. Yes, I do want to allow extensions to write any setting - given it exists. That is the foobar extension can reconfigure search/tabs/etc but it cannot introduce a new config option. When thinking of _key-value_-pairs, you can set the value of any key but you cannot introduce a new key. Only the static contribution point in package.json can do that.

Unrelated: would this API take into consideration that there are global settings and workspace settings?

Yes. Current thinking is that there WorkspaceConfiguration#update which takes the key, a value, and some sort of hint where to write to - which again is complex.

The problem with writing is that I'd like to know if for a given key, object is a valid value, as it is for search.exclude. That's cos generally I don't think it wise to have an object as arbitrary value, cos it would allow for config.update('search', {}) which results in an invalid config... Or on a second though, the config service (setWorkspaceConfiguration, setUserConfiguration) should maybe just reject changes that are not compliant to the schema. @aeschli Can we already do that?

[aeschli]

...reject changes that are not compliant to the schema

Right now the configuration service doesn't know anything about the configuration schemas, but it looks like it should.
It should know the real key names (a.b.c). When collecting the default values we should also collect the allowed types and enum values. There are more ways of defining restrictions with JSON schema, but that would require the full schema checking code that is now in the JSON extension.

[jrieken]

That would be deluxe ;-) Similar to other places in the API we would apply a change always locally, send it to the main side, and update the extension side according to what it did (we do the same for instance with editor selection).

[bpasero]

Yes, +1 for not introducing new keys to the setting. I also see the issue with validation so for sure, the API needs to run the same validation checks we see in the editor and reject invalid updates.

There is another complexity: today we have settings.json, tasks.json and launch.json that all go through the configuration service. We end up merging tasks.json and launch.json (actually any *.json) into the global config by taking the name of the file as the first part (so that debug can do getConfiguration("launch") and getting all the configured settings). If we allow to write, what happens if I use a key from task or debug land? It would need to end up in the right file. Alternatively we would have to prevent and reject writing to launch or debug at all.

The global vs workspace configuration change is tricky because an extension might only write to the global config but the user might have workspace config that actually overwrites this setting.

[jrieken]

The global vs workspace configuration change is tricky because an extension might only write to the global config but the user might have workspace config that actually overwrites this setting.

I knew that but the task.json and launch.json is new to my. My initial reaction is to not allow editing those via the API.

[bpasero]

@jrieken I pushed a first version of a new IConfigurationEditingService with the following API:

writeConfiguration(target: ConfigurationTarget, values: IConfigurationValue[]): TPromise<ConfigurationEditingResult>

Where

• target can be USER or WORKSPACE
• values is an array of key (string) value (any) pairs
• ConfigurationEditingResult possible error codes that can happen

Result codes:

• OK: configuration written to disk
• ERROR_UNKNOWN_KEY: attempt to write to a configuration key that is unknown
• ERROR_NO_WORKSPACE_OPENED: attempt to write to a workspace config without having a workspace opened
• ERROR_CONFIGURATION_FILE_DIRTY: attempt to write to a configuration that is currently edited by the user
• ERROR_INVALID_CONFIGURATION: attempt to write to a configuration that contains JSON errors

Open questions / issues

• it will not be possible to write to a configuration key from another extension that is not activated yet
• there is no schema validation for the values at the moment
• there is no backup/undo story at the moment

I wanted to get this started even with the remaining questions/issues still there so that we can think about a proper extension API for this feature.

[jrieken]

it will not be possible to write to a configuration key from another extension that is not activated yet

Why is that. aren't all keys and defaults and scheme known upfront when detecting package.json files?

[bpasero]

@jrieken you are right, I take it back. somehow I thought configuration contributions are only added to the registry when the extension gets activated. but they should get added once the extension host is up and running (for which the configuration registry is aware of and will update).

I have one thought on the possible error cases from the writeConfiguration call: Maybe we should not return some enumeration that an extension has to check for errors but rather actually return the promise in an error state. That would make sure that if someone calls writeConfiguration and continues after the promise, the code after the promise is not being executed but rather an error raised that has to be handled by the extension. Otherwise an extension might simply forget to handle such an error and assumes the configuration is written when in fact it is not.

Maybe it would be best to hook the error promise into the IConfigurationEditingService already so that normal workbench clients also do not fall into the same trap.

[jrieken]

Maybe we should not return some enumeration that an extension has to check for errors but rather actually return the promise in an error state.

That how I did it in the API sketch up: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/api/node/extHostConfiguration.ts#L61

[bpasero]

@jrieken makes sense, I think I should move this into the service so that we have consistent behaviour also for internal clients. If we do so, I could also provide a better error message so that the extension does not end up with the internal enumeration codes. As a last resort an extension could decide to show the error message to the user if wanted.

I think an extension itself should not react to the error cases in any other way (besides maybe showing the error) because the error cases can still change in the future and we do not want to make them API.

Thoughts?

[jrieken]

I could also provide a better error message so that the extension does not end up with the internal enumeration codes.

Agreed. I actually don't use the enum value but its name (providing a little bit of extra detail): https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/api/node/extHostConfiguration.ts#L61

The only thing to not forget is that rejections etc travel via IPC and must be JSONable (https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/api/node/mainThreadConfiguration.ts#L35)

[jrieken]

@bpasero What does happen with a 'setting' coming from task.json or launch.json? Is that a user setting and can it just be updated like a real setting?

[bpasero]

@jrieken I pushed a change to carry the error inside the promise as discussed. The error state of the promise has the shape:

IConfigurationEditingError
{
   code: ConfigurationEditingErrorCode
   message: string,
   toString: () => message
}

Currently this object travels to the extension, so the error code leaks a little bit. On the other hand, we do not make any statements about the shape of the error in the API either, so I think we are OK.

Regarding your question: good point. As it stands today, a call to writeConfiguration will fail with ERROR_UNKNOWN_KEY when trying to write a setting from tasks.json or launch.json. I think this is fair because we originally said we do not want to allow changing these settings files just yet.

Also, the way the editing service is implemented, it is not aware of any file besides settings.json in a workspace or settings.json globally.

[jrieken]

Regarding your question: good point. As it stands today, a call to writeConfiguration will fail with ERROR_UNKNOWN_KEY when trying to write a setting from tasks.json or launch.json. I think this is fair because we originally said we do not want to allow changing these settings files just yet.

My concern is that once we allow for that (updating of task.json/launch.json) what does global in the update call mean? Maybe it's better to not expose update like I have proposed now, but wire it up inside a richer object (maybe returned from inspect). Like so:

interface ConfigurationValue<V> {
  key: string;
  value: V;
  update(newValue: V): Thenable<void>;
  source: any;
  overrides?: ConfigurationValue<V>;
}

The key property would be the fully qualified name, like editor.tabSize, the source property needs refinement but the idea is that it will indicate where the value came from like settings.json, task.json, <schema> etc. The overrides property is the important bit as it points to another ConfigurationValue with _the same_ key. It is not applicable for all keys, like values from task.json et al. The longest chain I see is:

{ key: editor.tabSize, value: 4, source: workspace} 
   |
 overrides
   |
   V
{ key: editor.tabSize, value: 2, source: user} 
   |
 overrides
   |
   V
{ key: editor.tabSize, value: 8, source: schema}

The update call would always work in the scope of the value, meaning the effective value must not necessarily change.

[jrieken]

@isidorn requesting launch.json updates via the API

[jrieken]

related to/must fulfil requirements for: https://github.com/Microsoft/vscode/issues/9061

[bpasero]

@jrieken it would be easy for my writeConfiguration method to support task/launch.json, the ugly part is that it needs hardcoded knowledge about these 2 files which is odd because the concept of task and debug should not be wired into the configuration service. On the other hand, we already have a separate service for editing, so it is not super bad. The debt is to make the concept of task.json and launch.json pluggable into the configuration world.

Thoughts on writeConfiguration:

• you can only target WORKSPACE if any updated key starts with task. or launch.
• any key with task. or launch. will end up in the related task.json and launch.json with the first segment removed (e.g. task.foo => foo)
• I am likely not able to validate that the keys passed in are valid from the task/debug point of view so we would have to trust them

As for the inspect API I wonder what it means to have an update method on the ConfigurationValue that has source: schema? Because that value can only be readonly.

And then, how could an extension decide to write a setting to the WORKSPACE if no such setting exist yet?

I like the idea of making the target of the writeConfiguration call implicit from the extension API, however I do not think we can cover all use cases doing so easily.

[jrieken]

the ugly part is that it needs hardcoded knowledge about these 2 files

Apart from reading, how do their values end-up in the configuration service without their files naming being known?

[bpasero]

@jrieken also ugly, but less hardcoded: this all happens in https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/services/configuration/common/model.ts#L54 and the idea is that any JSON file in the .vscode folder is being looked at. Contents with settings.json are being put top level, any other file contents is being added under a key that matches the name of the file. If you have tasks.json with a key foo it ends up to be tasks.foo in the getConfiguration call.

[bpasero]

Rethinking how this could work for task and debug I think these are new ConfigurationTargets for the writeConfiguration call and should not be done in an implicit way: Debug and task should be able to tell the configuration editing service that there are new targets for configuration values by providing the full path to this file. When calling into writeConfiguration you can use the contributed targets as well as the built in ones (USER, WORKSPACE).

Now, from an extension point of view I also think that this should be made very explicit in the API: If an extension can change launch configurations, there should be a namespace in the API to do so (same for tasks). Imho we should not implicitly write to tasks.json or launch.json if the settings key starts with task. or launch.

@isidorn @weinand fyi

[isidorn]

Keys in the launch.json are contributed by each extension, vscode is aware of them by reading the package.json of that extension. So it does not make a lot of sense that VSCode should verify them since the extension contributes those keys in the first place.

[DanTup]

Quick question!

There are several comments in this thread that suggest we shouldn't be having settings that don't exist in the package.json. Currently we assumed this was a feature, eg. having "hidden" settings - we have a couple of settings that we (extension devs) turn on (such as logging traffic to the language service to a file, and I was thinking about feature toggles we don't want to show to users in the settings file/completion yet).

Are we just taking advantage of a lack of validation here and we should stop using settings that aren't declared in the package.json?

[bpasero]

@jrieken I added support to write to launch.json and tasks.json. Any configuration key that starts with launch.* or tasks.* ends up in those files in the workspace. There are some new behaviours:

• I throw a new error ERROR_INVALID_TARGET if someone tries to write to the global settings file when using launch.* or tasks.* as key
• there is no validation happening when using launch.* or tasks.* keys because those are not in our collection of configured schemas and default values
• I currently do not allow to replace the contents of the file (e.g. you cannot use launch as key and provide a rich object as the contents), so you always have to provide a key within the file. not sure if we need to revisit this based on the scenario from debug and tasks (ping @dbaeumer @weinand @isidorn). the workaround is to just call the write-API for each key you want to have in the file (though that does not allow you to remove any user defined setting)
• I currently also do not write to any other standalone settings file besides tasks.json and launch.json because this is very dangerous: an extension could decide to use other JSON files in .vscode folder to store data and we do not want to simply write to that file without knowing how the file is being used

Let me know if more is needed from the workbench.

[jrieken]

Any configuration key that starts with launch.* or tasks.* ends up in those files in the workspace.

Does it mean it's asymmetric? So when I have 'foo: true' in launch.json and launch.foo: false in settings.json. From my understanding the latter value (false) will be the effective value but it seems now that writing will change the other value. Is that correct/desired?

[bpasero]

@jrieken today it seems random who is winning in such a case. in my testing the 'launch.foo': false in settings.json is actually winning over the 'foo: true' in launch.json which seems somewhat unexpected.

I think when merging settings in our config model we should first take all settings from settings.json and then overwrite the ones found in any other JSON file.

[bpasero]

Pushed a change to ensure that any value in tasks.json or launch.json overwrites anything in settings.json under the same key.

[bpasero]

@weinand @isidorn @dbaeumer I pushed a change so that you can actually replace the entire contents when you use the tasks or launch key. This is only possible in those cases 👍

[isidorn]

@bpasero I just tried this and seems to work great! It would be lovely if we could format the launch.json / tasks.json / settings.json after editing it. Here is a small gif where I am adding a new configuration using the API after the hello world command

[bpasero]

@isidorn is that changing an existing launch.json or creating a brand new one?

[isidorn]

@bpasero that is changing an existing launch.json. When I create a brand new launch.json it is nicely formated.

[bpasero]

@isidorn actually I can reproduce and this seems to happen when you pass in a complex object to @aeschli method applyEdits https://github.com/Microsoft/vscode/blob/master/src/vs/base/common/jsonFormatter.ts#L34

The formatting seems to be ok'ish preserved if you just change simple key-value pairs, but not for such larger objects.

As a workaround you could just get the configuration and set it fully because then I am applying it using JSON.stringify().

[isidorn]

@bpasero thanks for the workaround!

[vilarinholeo]

Hi @bpasero or @isidorn ,

You can show me how you've solved the problem. In my extension I want to get all the keys from settings.json and replace where there is a flag of my extension. It is possible?