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

Representing WorkerLocation and WorkerNavigator similarly to other worker-exposed interfaces #9125

Closed
foolip opened this issue Feb 15, 2021 · 20 comments
Labels
data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API

Comments

@foolip
Copy link
Contributor

foolip commented Feb 15, 2021

The typical way that interface exposure is controlled is via Web IDL extended attributes like [Exposed=(Window,Worker)] on a single interface. However, WorkerLocation and WorkerNavigator and predate this mechanism, and are instead separate interface with a subset of the members. Navigator is only exposed in the window context and WorkerNavigator is only exposed in workers.

All members come via mixins, and because of this these members are only documented once on MDN, see for example https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/userAgent. Web developers would just use navigator.language or location.href in both window and worker scripts, so this seems appropriate.

In #8929 we're getting rid of mixins, and no doubt we'll find that that navigator.userAgent was supported earlier on Navigator than on WorkerNavigator. That's good, but the way window vs. worker support will be represented in BCD for these interfaces will be different than anything else. Maybe that's OK, but I want to raise the issue explicitly.

First raised in #9079 (comment).

@Elchi3
Copy link
Member

Elchi3 commented Feb 15, 2021

So, what are our options here? I guess the mixin guideline clashes with the worker guideline here.

It seems like we should proceed strictly per the new mixin guideline, so that we capture the correct data for both WorkerNavigator and Navigator. However, it does't make much sense to document these APIs twice (we don't do this when have [Exposed=(Window,Worker)]) either.

It is sad that specs aren't doing that here, but can we act as if this was done using that mechanism? What would that mean for BCD / MDN ?

I find the mixin guideline quite clear but maybe it is the worker guideline that is a bit underspecified? Is worker support clearly captured in BCD? I guess there is an issue somewhere and that needs solving first. Pointers appreciated.

@queengooborg queengooborg added the data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Feb 15, 2021
@foolip
Copy link
Contributor Author

foolip commented Feb 15, 2021

It is sad that specs aren't doing that here

It's not been done because there is some compat risk, but I've filed whatwg/html#6390.

For MDN/BCD, I think it'd be quite fine to document things as if there was a single interface, and note only somewhere on MDN that actually there's a WorkerNavigator interface object.

I don't think the worker guideline really works for communicating what things aren't supposed to be exposed in workers, since that would result in a lot of entries with all false values.

@ddbeck
Copy link
Collaborator

ddbeck commented Feb 15, 2021

I don't think the worker guideline really works for communicating what things aren't supposed to be exposed in workers, since that would result in a lot of entries with all false values.

This also clashes with omitting irrelevant features. Maybe BCD needs a shorthand for non-features ("this isn't supported and it isn't supposed to be supported")?

@foolip
Copy link
Contributor Author

foolip commented Feb 15, 2021

https://xhr.spec.whatwg.org/#xmlhttprequest is another situation like this, where [Exposed=Window] limits the exposure of a single responseXML attribute. However, it's worth noting that this is fairly rare, and is for pretty old-school parts of the platform, Navigator and XMLHttpRequest.

I would say that for the most part, what's needed is for MDN do make it clear whether an API works in workers or not. In terms of browser support, it's really only useful to note a lack of support, entries that amount to "was always there in workers as it should have been" or "was never in workers and shouldn't be" are pretty unhelpful.

@foolip
Copy link
Contributor Author

foolip commented Jun 11, 2021

This is blocking #8929 now, since demixing the remaining Navigator* mixins would result in duplicated entries.

It's possible the right answer here is to actually accept two BCD entries for these, and to link both BCD tables on the single MDN pages.

@ddbeck
Copy link
Collaborator

ddbeck commented Jun 23, 2021

OK, thinking on this a little bit today. Would this be a pretty comprehensive solution?

  1. Treat these like [Exposed=(Window,Worker)] with the exception of shallow entries for api.WorkerNavigator and api.WorkerLocation with notes saying, more or less, go look somewhere else for the members of this interface

  2. Revise the guidelines for worker support to say that a worker support entry should exist only if at least one browser triggers one or more of the following conditions:

    • Support for workers was added or removed after the introduction of the parent feature (i.e., support for workers is not coterminous with the parent feature)
    • By specification, the parent feature should have worker support, but it's not supported (i.e., failing to support workers when it should)
    • By specification, the parent feature should not have worker support, but it does support it (i.e., supporting workers when it shouldn't)

If that doesn't work, what's missing or what am I breaking?

@foolip
Copy link
Contributor Author

foolip commented Jun 23, 2021

That sounds almost perfect, but I have two points of doubt.

Treat these like [Exposed=(Window,Worker)] with the exception of shallow entries for api.WorkerNavigator and api.WorkerLocation with notes saying, more or less, go look somewhere else for the members of this interface

How can readers tell that navigator.clipboard is not supported in workers, and not supposed to be? It would look the same as navigator.userAgent.

By specification, the parent feature should not have worker support, but it does support it (i.e., supporting workers when it shouldn't)

Do you have an example or this? The only case I know of is https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseXML but that's sort of accidental and not information I think anyone can use.

@ddbeck
Copy link
Collaborator

ddbeck commented Jun 24, 2021

How can readers tell that navigator.clipboard is not supported in workers, and not supposed to be? It would look the same as navigator.userAgent.

My inclination is to treat this as a content problem for navigator.clipboard (and for navigator.userAgent, for that matter). At least on MDN, Exposed is a sort of pervasive problem in this way. For example, ReadableStream never says, in the body or in the compat table, where it's exposed.

By specification, the parent feature should not have worker support, but it does support it (i.e., supporting workers when it shouldn't)

Do you have an example or this? The only case I know of is https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseXML but that's sort of accidental and not information I think anyone can use.

No, not really. I was just trying to cover all the cases. I suppose we could just as easily say, "Never add a worker support entry, whether or not it has worker support, if it's specified as not having worker support." Though maybe that would be bad news if a feature is ever removed from workers only.

@teoli2003
Copy link
Contributor

I discovered this thread today.

I agree with it, with one difference: there are very few cases were there are different interfaces for the window case and the worker case: WorkerNavigator, WorkerLocation and the global scope (WorkerGlobalScope). They all have about 5-15 children (nothing like GlobalEventHandlers.

WorkerLocation is not even defined with a mixin in the spec (and has never been demixed on mdn/content).

In the spec, there are a few mixins related to Navigator and the global scopes, and, since #6637 landed, they are all but one "demixed". The only one that is not yet demixed is WindowOrWorkerGlobalScope: it is not too big (14 children) and easy to demix. I would add the special case of the console namespace as a 15th child here.

I think we should finish the demix here, as I see value of having "It is an interface, we have an entry, both in content and in bcd".

I agree there is a bit of redundancy here, but:

  1. Not that much, mostly on old useless features
  2. And this doesn't increase often (like maybe 1 new property to Navigator or WindowOrWorkerGlobalScopeevery couple of years.

For the [Exposed=()] cases, I agree, no separate entries, no duplicated content. That's 99% of the cases.

@foolip
Copy link
Contributor Author

foolip commented Jul 12, 2021

Just to be sure we're all working from the same understanding, here are the various interfaces and mixins at play:

  • Location+WorkerLocation and Navigator+WorkerNavigator are plain interfaces (not mixins) where the Worker* variants are only exposed in workers. This is a unique situation.
  • Various mixins like NavigatorID are included in both Navigator and WorkerNavigator.
  • Window and WorkerGlobalScope are real interfaces which the WindowOrWorkerGlobalScope mixin is included in, but isn't related to this issue.

I agree that the problem described in this issue is fairly small, but it's not "mostly on old useless features". New properties are added to navigator with some regularity, with navigator. userAgentData being the most recent I'm aware of.

The duplication on MDN seems especially unhelpful:
https://developer.mozilla.org/en-US/docs/Web/API/Navigator/permissions
https://developer.mozilla.org/en-US/docs/Web/API/WorkerNavigator/permissions

It is technically correct that these are separate interfaces, but it seems to me that at least on MDN it would be better to merge them. If not also merged in BCD, maybe that single page could include two compat tables.

My concrete suggestion is deleting api.WorkerNavigator.* and adding api.Navigator.*.worker_support where needed.

I don't think this needs to block demixing however, demixing NavigatorID and duplicating the data is worth it to get rid of mixins :)

@Elchi3
Copy link
Member

Elchi3 commented Jul 12, 2021

with navigator. userAgentData being the most recent I'm aware of.

The duplication on MDN seems especially unhelpful:
https://developer.mozilla.org/en-US/docs/Web/API/Navigator/permissions
https://developer.mozilla.org/en-US/docs/Web/API/WorkerNavigator/permissions

I've read in both specs that define these two features (userAgentData and permissions) and I don't understand why they are available to both Navigator and WorkerNavigator. So, I'd have trouble as well to document them separately in a way that makes sense to MDN readers. Is worker exposure just there because "it couldn't hurt" or is there some other reason? What practical advise could be documented on worker specific documentation pages to make the currently unhelpful pages more helpful?

If there is no reason, then I agree that separate pages are unhelpful as there isn't really anything to document separately other than "it works in both contexts", so api.Navigator.*.worker_support and single MDN pages seem like a better approach.

@foolip
Copy link
Contributor Author

foolip commented Jul 12, 2021

I don't understand why they are available to both Navigator and WorkerNavigator

In whatwg/html#6390 I speculated that it's just because these APIs were made available to workers before Web IDL had the [Exposed] extended attribute.

In that same issue it was pointed out that Location and WorkerLocation do have significant differences, where all properties are ready-only in workers but most can be set in the window context. This is reflected in https://developer.mozilla.org/en-US/docs/Web/API/WorkerLocation#properties it turns out.

For Navigator, I don't know of any differences in how one uses the API in window vs. worker contexts, and can't find any differences in how WorkerNavigator is extended by any spec. It's just that some things like navigator.plugins are not exposed to workers at all.

@foolip
Copy link
Contributor Author

foolip commented Jul 12, 2021

I've sent mdn/content#6818 to fix a bunch of things I found when looking at MDN pages around this issue.

@Elchi3
Copy link
Member

Elchi3 commented Jul 13, 2021

In that same issue it was pointed out that Location and WorkerLocation do have significant differences, where all properties are ready-only in workers but most can be set in the window context. This is reflected in https://developer.mozilla.org/en-US/docs/Web/API/WorkerLocation#properties it turns out.

Aha, okay. I guess that hints that we should rather have separate pages for Location and WorkerLocation so we document these clearly as separate features with their own characteristics like read-onliness?

For Navigator, I don't know of any differences in how one uses the API in window vs. worker contexts, and can't find any differences in how WorkerNavigator is extended by any spec. It's just that some things like navigator.plugins are not exposed to workers at all.

Seems like here we could then apply your suggestion to use api.Navigator.*.worker_support and single MDN pages. However, at this point, would that be the only exception we're making then?

@foolip
Copy link
Contributor Author

foolip commented Jul 13, 2021

In that same issue it was pointed out that Location and WorkerLocation do have significant differences, where all properties are ready-only in workers but most can be set in the window context. This is reflected in https://developer.mozilla.org/en-US/docs/Web/API/WorkerLocation#properties it turns out.

Aha, okay. I guess that hints that we should rather have separate pages for Location and WorkerLocation so we document these clearly as separate features with their own characteristics like read-onliness?

Yes, or to take the other side, it's important to have examples for Location of setting things like location.search and how that causes a navigation, and what setting location.hash will do (fires "hashchange" event). Those examples won't work in the worker context. If it's important to communicate that, it's actually not enough to just keep WorkerLocation separate, it also needs to be clear from https://developer.mozilla.org/en-US/docs/Web/API/Location/hash that it won't work in workers, since that page is what one will find when searching for location.hash.

I don't have enough experience with how web developers understand and misunderstand documentation to have an opinion about what we should do here.

For Navigator, I don't know of any differences in how one uses the API in window vs. worker contexts, and can't find any differences in how WorkerNavigator is extended by any spec. It's just that some things like navigator.plugins are not exposed to workers at all.

Seems like here we could then apply your suggestion to use api.Navigator.*.worker_support and single MDN pages. However, at this point, would that be the only exception we're making then?

Yes, if we don't merge Location and WorkerLocation, then I think we might just as well keep Navigator and WorkerNavigator separate as well. It is work to duplicate them, but it's also work to encode the exception in the Web IDL → BCD mapping everywhere that we'd end up with.

@Elchi3
Copy link
Member

Elchi3 commented Jul 13, 2021

I don't have enough experience with how web developers understand and misunderstand documentation to have an opinion about what we should do here.

I think a content issue would be great to get that figured out properly.

Yes, if we don't merge Location and WorkerLocation, then I think we might just as well keep Navigator and WorkerNavigator separate as well. It is work to duplicate them, but it's also work to encode the exception in the Web IDL → BCD mapping everywhere that we'd end up with.

Sounds good to me. I guess we have a resolution then? Is there work left in BCD? Looks like all 4 interfaces exist in the data already.

@foolip
Copy link
Contributor Author

foolip commented Jul 13, 2021

I've filed mdn/content#6856, closing this.

@foolip foolip closed this as completed Jul 13, 2021
@ddbeck
Copy link
Collaborator

ddbeck commented Jul 15, 2021

Is this fixed though? We don't have a guideline for how we're supposed to record this data in BCD and I'm not sure where that work would be tracked elsewhere.

@foolip
Copy link
Contributor Author

foolip commented Jul 15, 2021

The conclusion is to just not do anything special for WorkerLocation and WorkerNavigator, so it's resolved to my satisfaction.

@ddbeck
Copy link
Collaborator

ddbeck commented Jul 15, 2021

OK, great. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

No branches or pull requests

6 participants
@ddbeck @Elchi3 @foolip @teoli2003 @queengooborg and others