Document if null can possibly be returned

[lieser]

E.g. the following functions on specify promise of MailAccount>/MailIdentity, but return Promise<null> if the requested id does not exist.

await messenger.accounts.getDefaultIdentity("foo")
await messenger.accounts.get("foo")
await messenger.identities.get("foo")
await messenger.identities.getDefault("foo")

The above are just examples. There are probably more functions there it is currently unclear what happens in the error case. I.e. if a special value like null is returned, or if an exception is thrown, which is what I would expect if no special error value is documented.

[jobisoft]

These 4 functions state in the docs, that they return null if not found/defined. Other functions who throw, also have that stated in the docs. Do you have a case where the docs are wrong?

[lieser]

You are right, overlooked it in the description. And not, I don't know cases there this is missing int the description.

I still think it would be good to explicitly state it in the return type too. Not just to make it harder to overlook, but to also potentially allow generating type definitions from the same underlying data (see also #57). At least I assume that part in the doc is already automatically generated from some specification file.

[jobisoft]

correct, it is marked optional and the generator script could use that to add "or null" to the return type. will try that. https://searchfox.org/comm-central/source/mail/components/extensions/schemas/identities.json#125

[jobisoft]

We already use the notation to enclose a parameter in brackets [ ] to indicate it being optional. I could do the same with the return type:

Would this be an acceptable solution? The schema files have no way of defining the reason for it being optional, so we have to use the description to state the actual reason. Adding or null is therefore not possible, because it could be undefined as well, or it could throw.

Alternatively, we could add (optional) behind the type, but that would brake the used notation, I think.

[lieser]

I'm not 100% sure about the [...] notation, but with maybe some central documentation what this means, and the already available description, I think it could work out.

Adding or null is therefore not possible, because it could be undefined as well, or it could throw.

Note that the possibility of throwing is not something I would consider part of the return type. I would consider both distinguished parts of the functions signature. That the schema does not allow to be clearer about null/undefined is unfortunate, but nothing we can change.

The same syntax is used on types with optional values (e.g. Tabs). Does it mean that it can possibly be null there too? There is so far interpreted it as can possibly be undefined, and not that it can be null too. https://github.com/jsmnbom/definitelytyped-firefox-webext-browser seems to do the same. If my interpretation is correct, it would mean that [..] syntax has a different meaning for properties of types and return values, which is sup-optimal.

If the optional in the schema has a different meaning for return types and properties, I would go with (optional). If it means the same, I would probably go with [...]. And like I said at the beginning, separately document what this means exactly in regards to null/undefined. On could of course also explicitly add | undefined | null), but that is maybe a little to verbose.

I also had a look at what Firefox does (e.g. https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/cookies/get), but there the Return value is described just in a text.

Chrome is just writing optional (https://developer.chrome.com/docs/extensions/reference/cookies/#method-get). Don't know if they write somewhere if that mean null, undefined or both.

[JimDanner]

I think it would be better to put the alternative promise-values in the schemas explicitly. One reason is that formally, the type of an optional parameter param?: T is equivalent to T|undefined only, and not to T|null (if you don't believe that, try passing null into a TypeScript function that has an optional string argument). A second reason is that it's just clearer and easier to handle programmatically than having to rely on the documentation string. Thirdly, the schemas could use an update anyway – see the list of incorrect 'optional callback parameter' cases below.

Of course the description will still serve to explain the meaning and circumstances of a null or undefined value. This is already done well in the messageDisplay.json schema file. It lists the callback for the function messageDisplay.getDisplayedMessage with parameter type

"choices": [
  {
    "$ref": "messages.MessageHeader"
  },
  {
    "type": "null"
  }
]

and says in the description: "It returns null if no messages are displayed, or if multiple messages are displayed."

In the schemas as they are, a few problems occur:

1. Callback parameter listed as optional while null and undefined don't seem to be possible promise values
2. Callback parameter listed as required while null is actually a possible promise value
3. As stated above, no distinction between potentially returning undefined or null.

For the creation of @types/thunderbird-webext-browser I scanned the schema files and found the following cases:

Functions whose callbacks have an optional parameter according to the schemas:

Thunderbird API

• actually may return null:
  • accounts.get
  • accounts.getDefault
  • identities.get
• actually may return undefined:
  • mailTabs.getCurrent
  • tabs.getCurrent
• listed as optional but the documentation says nothing about that:
  • contacts.getPhoto
  • tabs.create, .duplicate, .update, .executeScript
  • windows.create

EDIT April 2024 – in the Thunderbird v127 JSON files, there are some more that are listed as optional while the documentation strings don't say anything about it:

• action.getLabel (from the file browserAction.json)
• composeAction.getLabel
• mailTabs.create, .update
• messageDisplayAction.getLabel
• sessions.getTabValue
• spaces.open
• spacesToolbar.addButton, .clickButton

Firefox API (the parts used in Thunderbird)

• actually may return null:
  • cookies.get and .remove
  • runtime.getBackgroundPage
• actually may return undefined:
  • alarms.get
• listed as optional but the documentation says nothing about that:
  • cookies.set
  • downloads.getFileIcon
  • identity.launchWebAuthFlow
  • webNavigation.getFrame and .getAllFrames

Firefox API (the parts not used in Thunderbird)

• listed as optional but the documentation says nothing about that:
  • devtools.inspectedWindow.eval
  • tabs.create, .duplicate, .update, .executeScript, .getCurrent
  • windows.create

Functions whose callbacks have no optional parameter but that can actually return null or undefined:

• identities.getDefault can return null according to documentation
• accounts.getDefaultIdentity can return null according to documentation

[jobisoft]

This is so awesome. Thanks for going through the schema files. I have a few other pending changes/fixes and I will incorporate your findings! Stay tuned, I will report back here.

[jobisoft]

I have updated our docs to include "optional" for optional input parameters/properties. https://webextension-api.thunderbird.net/en/latest-mv2/messages.html#messageattachment

Now I want to clean up the return types and include the alternative return values in our documentation as well.

I do not know why two of our APIs have been implemented to return undefined. Should we change that for MV3 and always return null if no value is to be returned, to be consistent? Or is undefined actually the better choice?

[JimDanner]

As I don't know why undefined was implemented in some returns, I can't tell with certainty. But it seems more reasonable that the return value would be null. Anundefined return value is equivalent to 'not doing return values' and null to 'returning a result that is empty'. When you do a query and there are no matching results, the value null makes most sense: the query has been executed correctly, it just matches zero ('null' = zero) results.

Looking at some of these cases:

• accounts.get returns null if "the requested account doesn’t exist"
• mailTabs.getCurrent returns undefined "if the active tab is not a mail tab"

And in the Firefox API:

• cookies.get(details) returns null "if the cookie was not found on the basis of details"
• alarms.get(name) returns undefined "If no alarms match name"

Well... those differences seem very tentative to me. In all cases, I think null would have been the best return value.

If a function returns undefined because of a malformed query, throwing an exception makes more sense: it makes clear that something really went wrong. If nothing went wrong but there was simply no result, null is better.

[jobisoft]

Ok, will change the two undefined cases to return null in MV3, after I have talked to the original developer who implemented them as such.

Can we establish that an optional return value is equivalent to returning null, so these two describe the same thing (and I do not have to change all schema files):

"choices": [
  {
    "type": "string
  },
  {
    "type": "null"
  }
]

"type:" string,
"optional": true

I will therefore explicitly list the undefined return value as a choice, so you can differentiate between null/optional and undefined.

I will include the optional return values also in the documentation, so all optional return values will get "or null" appended (or "or undefined" for the two special cases)

Will that solve the parsing issue on your side?

I of course still have to fix the schema files, which are wrong as you pointed out in this issue.

[JimDanner]

Can we establish that an optional return value is equivalent to returning null, so these two describe the same thing (and I do not have to change all schema files):

"choices": [
  {
    "type": "string
  },
  {
    "type": "null"
  }
]

"type:" string,
"optional": true

I think it is actually the other way around: optional parameter param?: T is equivalent with T|undefined. So if null is a possible result, it can only be put in the schema explicitly.

To see this, take a look at this TypeScript playground: as it is, this runs fine because undefined may be passed in as an optional argument value. If you uncomment line 7, an error occurs:

Argument of type 'null' is not assignable to parameter of type 'string | undefined'.

So I think it is better to make the null return values explicit (and, hopefully, stop returning undefined in all cases). Also, the callback argument then should not be optional (because optional would suggest it could return undefined). For example in accounts.get it would be changed from

"parameters": [
  {
    "$ref": "accounts.MailAccount",
    "optional": true
  }
]

to

"parameters": [
  {
    "choices":
    [
      {
        "$ref": "accounts.MailAccount"
      },
      {
        "type": "null"
      }
    ]
  }
]

Are you also removing the 'optional' flag in the case of all those functions, like downloads.getFileIcon, that never return anything like null or undefined? I think those functions should throw an error if they are used the wrong way, but it doesn't seem they can return undefined, and therefore their callback argument can't be optional.

[arndissler]

Normally, optional parameters defaults to undefined. So possible return values should be as explicit as possible. If a return value is truly optional, it can be undefined or a valid value.

Example // the result of identities.get

  {
    "type": "function",
    "name": "callback",
    "optional": true,
    "parameters": [
      {
        "$ref": "accounts.MailAccount",
        "optional": true
      }
    ]
  }

→ Result is either a MailAccount object or undefined. This doesn't align with the comment that the return value can be null or a valid object of the given type.

So I absolutely agree on changing the type of this (and the other definitions of type function and name callback whose result can be null) to:

  {
    "type": "function",
    "name": "callback",
    "optional": true,
    "parameters": [
      {
        "choices": [
          {
             "$ref": "accounts.MailAccount"
          },
          {
             "type": "null"
          }
      }
    ]
  }

This way all typings (even the Fx ones) can be patched without breaking changes, as it only narrows the result type to the more correct version.

[jobisoft]

I prepared a patch at: https://phabricator.services.mozilla.com/D208998

I did not change the return values, but tried to properly identify them in the schema. Copy-pasting the patch summary:

This patch updates the schema files to correctly specify the return values of Thunderbird's WebExtension APIs. This aims to help create proper TypeScript type definitions directly from the schema files.

There are 3 areas of interest:

The callback function parameter, which we use to define the return type is optinal. Our APIs can actually be used with callbacks, but they are of course optional. Some of our APIs did not specify them as optional. The schema validator ignored these issues and they went by unnoticed.

Functions can either return null or undefined and our APIs did both without explicitly specifying which. We now use optional: true to indicate undefined. If a function returns null, it is explicity stated as a possible return value.

Some of our WebExtensions APIs return objects with some/all of their properties being defined as optional. Some of those however have been returned as null, which is now fixed.

I hope I did not miss any, and this solves the reported issue.

Overall, undefined may be returned in:

• cloudFile.getAccount()
• cloudFile.updateAccount()
• mailTabs.getCurrent()
• sessions.getTabValue()
• tabs.getCurrent()

null may be returned in:

• ...action.getLabel()
• accounts.get()
• accounts.getDefault()
• accounts.getDefaultIdentity()
• contacts.getPhoto()
• identities.get()
• identities.getDefault()
• messageDisplay.getDisplayedMessage()

I think we have to stick with these design decisions. I will see if I can change the null values to undefined for Manifest V3.

[JimDanner]

Great. Are there any APIs that may return a completely empty promise? Or is that equivalent to undefined? For example, the callback that's the last argument to tabs.create(): its parameter, of type Tab, is listed as optional in the schema (and there's no clarification in its documentation). I listed a few more in my post above, probably not an exhaustive list (it looks like you found a few that I missed).

[jobisoft]

Great. Are there any APIs that may return a completely empty promise? Or is that equivalent to undefined? For example, the callback that's the last argument to tabs.create(): its parameter, of type Tab, is listed as optional in the schema (and there's no clarification in its documentation). I listed a few more in my post above, probably not an exhaustive list (it looks like you found a few that I missed).

I tried to fix all these. tabs.create() is no longer reporting that parameter as optional. It would be great if you could check if I missed anything.

If a method does not have any parameters in its callback, for example tabs.remove(), that is an undefined return value.

[jobisoft]

There is one exception in the subFolders property of the MailFolders type:

          "subFolders": {
            "description": "Subfolders of this folder. The property may be <value>null</value>, if inclusion of folders had not been requested. The folders will be returned in the same order as used in Thunderbird's folder pane.",
            "choices":[
              {
                "type": "array",
                "items": {
                  "$ref": "folders.MailFolder"
                }
              },
              {
                "type": "null"
              }
            ],
            "optional": true
          },

It must be optional, because we used to use the MailFolder as a partial input (path & accountId), but it is returned as null, and I think I cannot change that without breaking extensions. The use as partial input is deprecated, but that needs to be kept for the next ESR.

[jobisoft]

Fixing the remaining issues in:

• https://bugzilla.mozilla.org/show_bug.cgi?id=1915253
• https://bugzilla.mozilla.org/show_bug.cgi?id=1915259