w3c / webextensions

Charter and administrivia for the WebExtensions Community Group (WECG)
Other
575 stars 50 forks source link

`ExecutionWorld` and `StyleOrigin` should use lowercase values #563

Open xeenon opened 3 months ago

xeenon commented 3 months ago

At present, ExecutionWorld requires values to be uppercase, specifically MAIN or ISOLATED. Similarly, StyleOrigin also requires the uppercase values AUTHOR or USER. This approach, for these newer APIs, is in contrast to the convention used for older properties, such as RunAt, which uses lowercase values like document_start or document_idle.

To improve consistency across the Web Extensions API, I recommend standardizing in favor of lowercase for these values. Additionally, for backward compatibility, browsers should adopt a case-insensitive comparison for these values until the uppercase values are sufficiently phased out.

Moving forward, I suggest the adoption of lowercase string constants in all future APIs, to establish a uniform standard.

erosman commented 3 months ago

Original MV2 tabs.insertCSS(): cssOrigin & CSSOrigin also used lowercase "user" & "author".

rdcronin commented 3 months ago

I agree with deciding on a uniform standard.

ExecutionWorld is not the first (or by any means, only) API to use SCREAMING_STYLE for the enum values. We have historically been very inconsistent about this, using combinations of SCREAMING_STYLE, camelCaseStyle, and snake_case_style. We eventually somewhat standardized in Chrome on SCREAMING_STYLE because these were similar to enums or constants, both of which are represented with SCREAMING_STYLE in the Google JS Style Guide, and is also the referenced "common convention" in MDN to represent constants -- though of course, MDN would never require a certain style.

I would prefer aligning on using all-caps to better fit with these existing patterns and conventions.

Note also that case-insensitivity only works between SCREAMING_STYLE and snake_case_style; it does not solve the issue for camelCaseStyle. Several years back, to help avoid this issue, Chrome began exposing enums on the APIs themselves, a la chrome.runtime.ContextType.BACKGROUND -- these are always SCREAMING_STYLE, even when the mapped string value is not (chrome.runtime.RequestUpdateCheckStatus.NO_UPDATE evaluates to 'no_update').

tophf commented 3 months ago

I would prefer aligning on using all-caps to better fit with these existing patterns and conventions.

No, you've introduced an inconsistency because values in the API have always been in lowercase/camelCase. Even the new declarativeNetRequest API uses lowercase/camelCase for the values e.g. "type" : "block". You also weirdly equate the rules for naming of a constant to its value, which is not regulated by Google JS Style Guide you've linked, AFAICT, or any other JS guide I know for that matter.

xeenon commented 3 months ago

@rdcronin I agree SCREAMING_STYLE should be used for the JS properties representing the constants, and having the string values be lowercase.

dotproto commented 3 months ago

@rdcronin I agree SCREAMING_STYLE should be used for the JS properties representing the constants, and having the string values be lowercase.

I read @rdcronin's comment as suggested SCREAMING_STYLE for enums and constants, and suggesting that we align on uppercase to match Chrome's current conventions.

You also weirdly equate the rules for naming of a constant to its value, which is not regulated by Google JS Style Guide you've linked, AFAICT, or any other JS guide I know for that matter.

That doesn't seem odd to me at all. Automated tooling can easily explain how coding style can directly impact implementation details. For example, TypeScript enums auto-assign numeric and string properties based on the order of member declaration and the member's key.

// TypeScript source
enum Shape {
    CIRCLE,
    SQUARE,
}

// Compiled JS
var Shape;
(function (Shape) {
    Shape[Shape["CIRCLE"] = 0] = "CIRCLE";
    Shape[Shape["SQUARE"] = 1] = "SQUARE";
})(Shape || (Shape = {}));

console.log(Shape[0], Shape.CIRCLE); // "CIRCLE"
console.log(Shape.CIRCLE) // 0
tophf commented 3 months ago

suggesting that we align on uppercase to match Chrome's current conventions.

It doesn't seem practical or reasonable, because just a handful of values are in uppercase in a couple of new API, whereas hundred(s) of the values are in lowercase.

That doesn't seem odd to me at all. Automated tooling can easily explain

Linking a style guide for naming was unequivocally odd. Your argument doesn't stand either as automated tools can easily do whatever we decide, i.e. automation is irrelevant, moreover Chromium already has the tooling to generate the uppercase names from lowercase values.

I'd also like to point out that despite @rdcronin's comment this issue is not about "impassioned bikeshed discussions". It's about an objectively existing problem introduced by chromium team, apparently while trying to improve consistency but ending with an anecdotally (xkcd) new inconsistency.

Rob--W commented 3 months ago

https://github.com/w3c/webextensions/labels/supportive%3A%20firefox for having consistency across APIs. The specifics (i.e. whether lowercase or uppercase) is a lesser concern.

tophf commented 3 months ago

The specifics (i.e. whether lowercase or uppercase) is a lesser concern.

From a practical point of view, this concern is just as important, because if the decision will be to use the uppercase for values it would mean changing a lot of existing values (possibly hundreds) across multiple APIs, which seems entirely unrealistic. ~I do understand that expecting chromium team to admit they made a mistake is not entirely realistic either.~ (Edit: looks like it's not that clear-cut). The most realistic solution would be to ignore the problem, of course.

Rob--W commented 3 months ago

An easily actionable task here is to reach consensus on the supported value format for new APIs.

Secondarily, we could consider changes to existing APIs that deviate from the common conventions, but I wouldn't require that.

rdcronin commented 3 months ago

Just to be clear: we have never been consistent; this wasn't something that Chrome just recently changed. Uppercase values were introduced more than a decade ago (one easy example).

Today, non-trivial numbers of enum entries are:

Less common variations include enums with hyphens (8) and enums with number (12).

The web itself is also inconsistent. While many favor lowercase (and there, they use a lot more hyphens than underscores), there are also examples of uppercase enums (e.g., GET and POST). However, most browsers today allow case-insensitive comparison of these, which is inline with @xeenon 's recommendation in the issue summary.

An easily actionable task here is to reach consensus on the supported value format for new APIs.

Agreed. @xeenon , do you mind if we repurpose this issue's title to target that?

tophf commented 3 months ago

Okay, so Chromium did a sensible thing to always name them in SCREAMING_CASE in those original IDLs, it's not what the issue is about and anyway those historically uppercase values are scattered in rarely used APIs/properties, so my earlier claim should be corrected to say that almost all APIs used by most extensions have all their values in lowercase/camelCase:

Isn't it obvious that a lowercase value is the most rational choice?

Note how runtime had 6 lowercase enums with dozens of lowercase values and suddenly it receives a new ContextType enum with all uppercase values, which looks objectively weird, unusual, and wrong.

Arguably, if we scan the source code of all existing extensions for literal enum values like "document_start" and exclude the recently introduced uppercase values, close to 99% of them will be lowercase.

dotproto commented 2 months ago

While looking into WebIDL for unrelated reason, I came across the following quotation after the definition of enumeration values:

Warning!

It is strongly suggested that enumeration values be all lowercase, and that multiple words be separated using dashes or not be separated at all, unless there is a specific reason to use another value naming scheme. For example, an enumeration value that indicates an object should be created could be named "createobject" or "create-object". Consider related uses of enumeration values when deciding whether to dash-separate or not separate enumeration value words so that similar APIs are consistent.