-
Notifications
You must be signed in to change notification settings - Fork 523
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
+55
−24
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
eaab1bd
Add support for more worker cases in {{AvailableInWorkers}}
teoli2003 110ed7d
Add missing full stop
teoli2003 20061f5
Add support for 'notservicenotwindow'
teoli2003 b9c41c1
Apply review comments
teoli2003 c1232e5
Second attempt
teoli2003 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 nameddedicated_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".
There was a problem hiding this comment.
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
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?
There was a problem hiding this comment.
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:
dedicated_and_shared
)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)
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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!!!