Might also be worth to add a API-lint rule for this

These have subtly different meanings:

• property?: type — You can simply omit the property / argument. You can also pass in undefined if you want to be explicit

• property: type | undefined — The must explicitly specify the property or argument by setting it to undefined

We can review places where these occur in our d.ts, but coming up with a lint rule for this is probably not possible

With TS 4.4, there's another complication: Exact Optional Property Types

This means that ? is no longer automatically equivalent to | undefined

Here's a draft of guidance around this:

For function/method parameters:

• For implementation, treat omitting a parameter with ? the same as explicitly passing in undefined
• Use | undefined when you want to callers to always have to consider the parameter.
• Use ? when you want to allow callers to omit the parameter.
• Never use ? and | undefined on a parameter. Instead follow the two rules above to decide which version to use
• If adding a new parameter to an existing function, use ? as this allows the new signature to be backwards compatible with the old version
• Do not add an overload to add an optional parameter to the end of the function. Instead use ?

For properties

• Do not write code that treats the absence of a property differently than a property being present but set to undefined

  • This can sometimes hit you on spreads or iterating through objects, so just something to be aware of

• For readonly properties on interfaces that VS Code exposes to extensions (this include managed objects, as well as the objects passed to events):

  • Use | undefined as this makes it clear the property exists but has the value undefined.

• For readonly properties on options bag type objects passed from extensions to VS Code:

  • Use ? when it is ok to omit the property
  • Use | undefined when you want the user to have to pass in the property but undefined signals that you will fall back to some default
  • Try to avoid ? + | undefined in most cases. Instead use ?. Using both ? + | undefined isn't wrong, but it's often more clear to treat omitting the property as falling back to the default rather than passing in undefined

• For unmanaged, writable objects:

  • If using ?, always also add | undefined unless want to allow the property to be omitted during initialization, but never allow users to explicitly set it to undefined afterwards. I don't think we have many cases where this will be needed
    • In these cases, you may want to try changing the api to avoid this potential confusion
  • If adding a new property to an unmanaged object, use ? as this ensures the type is backwards compatible with the old version

For November, let's review classes that use readonly property?: type. I think almost all of these should be changed to readonly property: type | undefined instead because this makes it more clear that the property exists but is set to undefined.

Assigning owners per class:

• ThemeIcon @aeschli
• EvaluatableExpression @weinand
• InlineValueVariableLookup @weinand
• InlineValueEvaluatableExpression @weinand
• SemanticTokens @alexdima
• SemanticTokensEdits @alexdima
• SemanticTokensEdit @alexdima
• LinkedEditingRanges @aeschli
• StatusBarItem @bpasero
• TaskGroup @alexr00
• Task @alexr00
• DebugAdapterServer @weinand
• Breakpoint @weinand
• TestRunRequest @connor4312
• TestRun @connor4312
• TestItem @connor4312

Let me know if you have any questions about fixing these cases

@mjbvz IIRC I did mine a couple of weeks ago. Is there something more to do?

With exact-optional-property-types extension authors will have to provide all properties of type readonly property: type | undefined.
Wouldn't that be a breaking change?

I think it makes sense to keep readonly property?: type for optional properties.

Since the constructors of all my assigned classes are public, changing readonly property?: type into readonly property: type | undefined would be breaking. So no changes on my side.

@aeschli Not if the type is maintained by VS Code. It would be breaking if you make this change to an options bag object

@weinand You should not change the constructor arguments but change the property. For example, DebugAdapterServer

	export class DebugAdapterServer {
		readonly port: number;
		readonly host?: string;
		constructor(port: number, host?: string);
	}

Should become:

	export class DebugAdapterServer {
		readonly port: number;
		readonly host: string | undefined;
		constructor(port: number, host?: string);
	}

Using | undefined is more consistent in this case and also matches how these classes are implemented:

@weinand and @aeschli Can you please quickly revisit your classes and update any cases that are similar to the above example

@mjbvz sorry Matt that I did not made myself clear:
since the constructor is not private, objects of class DebugAdapterServer can be created as { port: 1234 } (which is actually done in some extensions). Changing host?: string to host: string | undefined breaks that code.
This is true for all my other cases...

@mjbvz Same for ThemeIcon, const themeIcon : ThemeIcon = { id: 'zap' }; would no longer compile.
If we don't encourage creating objects like that, I'll make the change (or feel free to make it yourself)

Thanks for the additional context

With typescript in general, if something is typed as class, we expect instances of that class. If you only care about the properties, use interface instead. However we do often use classes + constructors as quick ways to create simple objects with a few properties in vscode.d.ts, so we are not following these rules all the time

A few options for these cases:

1. If you want extensions to always create an instance of the class, change the properties to readonly prop: type | undefined

2. If you instead want to allow extensions to pass in objects with the same shape as the class, change the property on the class to: readonly prop?: type | undefined. That is ugly but I think makes the types correct for strictOptionalProperties

These change will not break the runtime behavior of extensions. However option 1 may break compilation for extensions that don't create instances of the class

Thanks @mjbvz for looking into this and coming up with guidelines.
I went with color?: ThemeColor | undefined; for ThemeIcon as I expect some extensions to use the { id: 'zap' } notation. We happen to do it in our sample.
For LinkedEditingRanges I went with the recommended way.
Feel free to change this if you prefer to have this consistent.

@mjbvz thanks for your suggestions.

Since option 1 is breaking, we cannot use it (even if we would like to encourage users to always create class instances). So I've applied option 2 in all my cases.

Thanks. I'm going to close this and move the API guidance to a wiki

I believe vscode.d.ts mostly gets strictOptionalProperties correct now. We can handle any other issues with it on a case by case basis since making a complete and thorough pass through the api doesn't seem worthwhile at this time