1785284 - Decide style guide for enum type definitions in remote/

Status: NEW

(Remote Protocol :: Agent, task, P3)

Description

[Julian Descottes [:jdescottes]]

The current way we write type definitions for enums is not 100% consistent, triggers errors in VSCode and does not follow typical style guides for global constants.

Some examples: https://searchfox.org/mozilla-central/rev/f3616b887b8627d8ad841bb1a11138ed658206c5/remote/webdriver-bidi/modules/root/browsingContext.jsm#41

/**
 * @typedef {Object} CreateType
 **/

/**
 * Enum of types supported by the browsingContext.create command.
 *
 * @readonly
 * @enum {CreateType}
 **/
const CreateType = {
  tab: "tab",
  window: "window",
};

The issue here is that we create a typedef named CreateType, but then we also create a const called CreateType. This confuses VSCode which considers it as a duplicate declaration. To avoid this, the const should use a different name. For instance CREATE_TYPE (or rather CREATE_TYPES, if we don't reuse exactly the type name, I think the plural makes more sense?).

We should then make sure all enum typedefs follow the same pattern under remote/. We don't have a lot of them: https://searchfox.org/mozilla-central/search?q=%40enum&path=remote&case=false&regexp=false (11 at the time of filing).

While doing this we should take the chance to rename the keys of those enums to be all uppercase. This is how common styleguides suggest to expose global constants and it makes it easy to spot constant usage when reading the code.

Going back to the example above, the new version could be

/**
 * @typedef {Object} CreateType
 **/

/**
 * Enum of types supported by the browsingContext.create command.
 *
 * @readonly
 * @enum {CreateType}
 **/
const CREATE_TYPES = {
  TAB: "tab",
  WINDOW: "window",
};

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Julian! I wonder if we should make an exception for the actual enum name. Even as used as a global constant it might still be better to use camel-case like naming so it's somewhat close to how class names are used? Not sure if that is a good argument, but maybe we could have a look how Typescript is actually doing that so in case that we might use it in the future there is lesser work to do?

Otherwise it looks fine to me.

Comment 2

[Julian Descottes [:jdescottes]]

Still need to agree on the exact pattern but should be a straightforward bug after that.