yari icon indicating copy to clipboard operation
yari copied to clipboard

feat(macros/AvailableInWorkers): support more distinct cases

Open teoli2003 opened this issue 2 years ago • 9 comments

Summary

We have the {{AvailableInWorkers}} macro that adds a message indicating a feature is available in web workers.

It was created a long time ago (when workers were added to the web platform) and supports only two cases:

  • all web workers (and Window) support the feature;
  • all web workers support the feature, but service workers (and Window).

Over the years, many other cases were added. I've found cases where a given feature is supported:

  • only in a DedicatedWorker (and in Window)
  • only in a DedicatedWorker (even not Window)
  • only in a ServiceWorker (even not Window)
  • only in ServiceWorker (and in Window)

This PR adds these few cases.

Fixes #10009

Problem

We have content cases where no adequate message can be displayed: E.g., https://github.com/mdn/content/pull/30191#issuecomment-1808631489. Many (read: ~50) occurrences will use it (either the macro call is missing or the text shown is not correct)

This PR fixes it by allowing more strings in parameters.

Solution

The macro now tests more argument values and sets the correct text for each of them.

In the long term, we may want to read this info from w3c/webref, but there are many more global scopes (Worklets, JsonLD, RTCIdentityProvider, InterestGroupScriptRunnerGlobalScope…), so it would mean renaming this macro (for the least). A bit overkill for the current needs.

Screenshots

I've added the macros in a file with the different messages: Capture d’écran 2023-11-15 à 12 45 52 And checked the result: Capture d’écran 2023-11-15 à 12 31 19


How did you test this change?

Manually tested (see screenshot)

teoli2003 avatar Nov 15 '23 11:11 teoli2003

cc/ @bsmth

teoli2003 avatar Nov 15 '23 11:11 teoli2003

Oh super, that's pretty cool, tnx. Would be interesting to get feedback from other there, but looks good to me :+1:

bsmth avatar Nov 15 '23 16:11 bsmth

A point that I forgot to mention: the macro stays compatible with what exists today; it can be merged without simultaneous changes to mdn/content.

teoli2003 avatar Nov 15 '23 17:11 teoli2003

maybe we could add one more case, for example, using the notserviceonly to suggest that all web workers support the feature, but service workers (event not Window)

a possible example is FileReaderSync interface

skyclouds2001 avatar Nov 16 '23 10:11 skyclouds2001

Good catch! I added it under the name notservicenotwindow (I found notserviceonly not clear).

Capture d’écran 2023-11-16 à 11 53 36 Capture d’écran 2023-11-16 à 11 53 20

With the new structure, it is quick to add new cases.

teoli2003 avatar Nov 16 '23 10:11 teoli2003

maybe we could add one more case, for example, using the notserviceonly to suggest that all web workers support the feature, but service workers (event not Window)

a possible example is FileReaderSync interface

I was going to tag you in a related PR to let you know about these changes, but you're already on it :)

bsmth avatar Nov 16 '23 13:11 bsmth

I apply @caugner proposal, which is better:

  • only_dedicated_and_window
  • only_dedicated
  • except_service
  • except_service_and_window
  • only_service_and_window
  • only_service

It isn't compatible with the current parameter (notservice), but this can be fixed quickly (a find and replace.

Internally, I kept the complete sentences (not following yin1999 request) as 1) it is a non-trivial change, 2) we will need to be redone anyway when we will do a more generic macro ({{Available}}) that can handle all cases of the Exposed= WebIDL extended attribute 3) few (if any) extra cases are expected in the mid-term (2023-2024).

teoli2003 avatar Nov 20 '23 13:11 teoli2003

What about the following? (The commit c1232e5 implements it):

From: // 'only_dedicated_and_window': only in DedicatedWorker (and in Window) // 'only_dedicated': only in DedicatedWorker // 'except_service': all workers but ServiceWorker (and in Window) // 'except_service_and_window': all workers but ServiceWorker (and no window) // 'only_service_and_window': only in ServiceWorker (and in Window) // 'only_service': only in ServiceWorker // null: (default) All workers (and Window)

To: // 'window_and_dedicated': only in DedicatedWorker (and in Window) // 'dedicated': only in DedicatedWorker // 'window_and_worker_except_service': all workers but ServiceWorker (and in Window) // 'worker_except_service': all workers but ServiceWorker (and no window) // 'window_and_service': only in ServiceWorker (and in Window) // 'service': only in ServiceWorker // null: (default) All workers (and Window)

The current {{AvailableInWorker("notservice")}} must be changed to {{AvailableInWorker("worker_except_service")}} (Follow-up mdn/content PR if this one is approved)

teoli2003 avatar Dec 11 '23 08:12 teoli2003

Is it still working in progress?

skyclouds2001 avatar Mar 13 '24 10:03 skyclouds2001