Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(macros/AvailableInWorkers): support more distinct cases #10029

Merged
merged 5 commits into from
Mar 28, 2024

Conversation

teoli2003
Copy link
Contributor

@teoli2003 teoli2003 commented Nov 15, 2023

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., mdn/content#30191 (comment). 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 teoli2003 requested a review from a team as a code owner November 15, 2023 11:53
@github-actions github-actions bot added the macros tracking issues related to kumascript macros label Nov 15, 2023
@teoli2003
Copy link
Contributor Author

cc/ @bsmth

@caugner caugner added the involves: Content Requires the attention of the Content team. label Nov 15, 2023
@bsmth
Copy link
Member

bsmth commented Nov 15, 2023

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

@teoli2003
Copy link
Contributor Author

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.

@skyclouds2001
Copy link
Contributor

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

@teoli2003
Copy link
Contributor Author

teoli2003 commented Nov 16, 2023

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.

@bsmth
Copy link
Member

bsmth commented Nov 16, 2023

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 :)

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, going to leave a +1 on this. Will not merge yet in case anyone else has comments or feedback. 👍🏻

Copy link
Member

@yin1999 yin1999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those strings are really similar, is it possible that we can set up some template for them?

Note, we need to localize both plain texts ("This feature is available in ${{place holder}}", ...) and link texts ("Web Workers", "Service Workers", etc. For zh-CN and zh-TW locales, we will remove the tailing "s")

Comment on lines 7 to 12
// 'dedicated': only in DedicatedWorker (and in Window)
// 'dedicatedonly' only in DedicatedWorker
// 'notservice': all workers but ServiceWorker (and in Window)
// 'notservicenotwindow': all workers but ServiceWorker (and no window)
// 'service': only in ServiceWorker (and in Window)
// 'serviceonly': only in ServiceWorker
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have a more generic/systematic interface?

{{AvailableInWorkers("only", "dedicated,window"}}
{{AvailableInWorkers("only", "dedicated"}}
{{AvailableInWorkers("except", "service"}}
{{AvailableInWorkers("except", "service,window"}}
{{AvailableInWorkers("only", "service,window"}}
{{AvailableInWorkers("only", "service"}}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, I would prefer to have the keys a bit more verbose:

only_dedicated_and_window
only_dedicated
except_service
except_service_and_window
only_service_and_window
only_service

@caugner caugner changed the title Add support for more worker cases in {{AvailableInWorkers}} feat(macros/AvailableInWorkers): support more distinct cases Nov 17, 2023
@teoli2003
Copy link
Contributor Author

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).

// '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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@teoli2003 Isn't this the same as only_dedicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there can be a SharedWorker too: https://developer.mozilla.org/en-US/docs/Web/API/SharedWorker

(At least it is my understanding)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

except_service_and_window = DedicatedWorker + SharedWorker
only_dedicated = DedicatedWorker

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification, so this sounds like except_service_and_window could also be named dedicated_and_shared?

Wouldn't it be easiest to just list the workers in which the feature is available (e.g. dedicated,shared) and display a comma separated list of worker types where the feature is available?

Looking at the screenshots again, I find it confusing that there is "available in Dedicated Web Workers" and "only available in Dedicated Web Workers".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caugner it sounds like this is hard because we can't easily specify multiple options in a macro. Why don't we just do the same as compat/specs and pull the information from metadata? Then we can make the support explicit:

worker support

  • window
  • dedicated worker
  • shared worker
  • service worker

Then {{AvailableInWorkers}} renders some standard text then "Supported in the following types of workers: [list]". If no metadata specified we could assume "all".

Would that be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get the info from w3c/webref. Nevertheless, there is one caveat the website equivalent for {{AvailableInWorker}}: it is more generic as would really be an {{Availability}} macro. There are many more cases: it would tell us if something is available in Worklet (of all kinds), in RTC's Identity provider, …

I think the mid-term solution would be to ditch {{AvailableInWorker}} for an {{Availability}} macro that we can put everywhere (like {{Specifications}} and {{Compat}}). I didn't do this here as this is a much more significant chunk of work (because there are many more cases, and it has to work with additional ones that can appear in the future, and because we need to obtain the w3c/webref key – again not really difficult but a more significant chunk of work).

That's why I wanted a short-term, more effortless solution (once we have the mid-term solution, a script can be run to update all the pages).

From the two proposals from @caugner:

  • more explicit text (dedicated_and_shared)
  • list of features

I prefer the first one as we don't need to add some parsing (or a loop with many unused cases) to the code. The only caveat is that it won't be compatible with the current arguments (which is why I chose the current keywords), so we need a find and replace PR on the whole MDN once this lands (not difficult).

It is on my todo list to update this PR, hopefully today (but so many things are happening in parallel, so that's not a promise)

Copy link
Contributor

@hamishwillee hamishwillee Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I much prefer my proposal of using metadata. This #10029 (comment) is overly complicated because you're trying to define exceptions.

I'm not sure about "one {{Availability}} macro to rule them all". If we have stuff in metadata I think we can do away with macros altogether at some point.

EDIT PS, though I guess I'd like this in "as is" to never getting agreement :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the mid-term, we won't even need metadata as this information is available via w3c/webref, which we already use in some macros. (But that's a significant chunk of work, and we need both some writers to define where and how (=text) to display on the page and non-trivial reviewing of the new macro).

That's why I would prefer this stop-gap first: it is similar to what we do know and doesn't impair the change in the future (a find-and-replace script will be easy to do)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Then how do we get this in!!!

@teoli2003
Copy link
Contributor Author

teoli2003 commented Dec 11, 2023

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)

@skyclouds2001
Copy link
Contributor

Is it still working in progress?

@github-actions github-actions bot removed the idle label Mar 15, 2024
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTU (@argl @fiji-flo @caugner), thanks! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
involves: Content Requires the attention of the Content team. macros tracking issues related to kumascript macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: make {{AvailableInWorkers}} macros support to specific worker type
6 participants