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

API structure conventions: mixins #1940

Closed
Elchi3 opened this issue Feb 1, 2021 · 18 comments
Closed

API structure conventions: mixins #1940

Elchi3 opened this issue Feb 1, 2021 · 18 comments
Assignees
Labels
MDN:Project Anything related to larger core projects on MDN

Comments

@Elchi3
Copy link
Member

Elchi3 commented Feb 1, 2021

Here we are again talking about mixins :) There isn't actually an issue on mdn/content yet that will formally decide about mixins, so I'm opening this one. Fun fun fun!

Please also read mdn/browser-compat-data#472 for prior discussions.
And if you are concerned about how inheritance structures (as opposed to mixins) are documented, then please read #1006.


So, observing mixins in the wild and deciding how to document them. I recently chatted to Rachel Andrew about this and here's what my take has been:

The idea is to document Aria attributes. A spec for them is here https://www.w3.org/TR/wai-aria-1.2/#AriaAttributes and the idl looks like this:

interface mixin AriaAttributes {
  attribute DOMString? ariaActiveDescendant;
  attribute DOMString? manymoreattributes;
}

Element includes AriaAttributes;

Now, as an MDN writer who uses WebIDL heavily to determine how and where to document things, you are not given a guideline where to document ariaActiveDescendant and friends, and so there are two options basically:

  1. Create a new API tree called AriaAttributes and put it there: docs/Web/API/AriaAttributes/ariaActiveDescendant
  2. Create a new page under the Element page tree: docs/Web/API/Element/ariaActiveDescendant

Option 1. exposes the mixin to the world of web developers and option 2. hides this abstraction.

I recommended option 2. and so why did I do that?

  • Mixins are abstractions that aren't exposed or observed directly. They are specification or browser-engine implementation helpers.
  • Browsers and specs use mixins to make their life easier and they rename, remove, re-organize them at their discretion. Documentation can't easily follow this. Example in case: In a later spec (https://rawgit.com/w3c/aria/master/#ARIAMixin), there is no AriaAttributes anymore, it is now called ARIAMixin. Consequently, with option 1. the page would need to move to docs/Web/API/ARIAMixin/ariaActiveDescendant and BCD would need to be updated, etc. But why? It doesn't really matter to web developers because Element.ariaActiveDescendant is going to be there no matter where in the spec or in IDL it has been defined.

On MDN, we can find API docs that do not hide the mixins however. Why is that? Example:

So, I guess the idea here is that mixins add features to multiple interfaces and so having to duplicate docs for every interface they appear on, should be avoided. Does that make sense, though? Mixins can disappear and be renamed still. Even in this case the docs make it quite confusing to the readers.


I'll stop here as it introduces the initial problem space. In a follow-up, I'll try to get to the cases where hiding mixins would mean a lot of (duplicated) content. I want to understand more how these duplications would look like and if it is really duplication or docs in different contexts.

Feedback welcome.

@dontcallmedom
Copy link
Contributor

My own natural inclination is the same as your, option (2) above; I guess the questions of duplication of content will be key to figure out implementability though.

@jpmedley
Copy link
Collaborator

jpmedley commented Feb 1, 2021

I have always been an advocate of option 2. (I should note that while Chrome does use mixins in our IDL, we don't always use the ones listed in web specs. Rachel and I bumped into an example of this last week, which I can't seem to find at the moment.)

It seems to me that to do this properly, there should be some kind of inclusion mechanism for both MDN pages and BCD. I don't know what that needs to look like.

@Ryuno-Ki
Copy link
Collaborator

Ryuno-Ki commented Feb 1, 2021

Hm, I searched Microsoft/TypeScript repo but couldn't find any reference there.
I wonder how they implement it.
Alternatively: Does Facebook's Flow has support for this ootb?

@hamishwillee
Copy link
Collaborator

hamishwillee commented Feb 1, 2021

So requestAnimationFrame and cancelAnimationFrame used to be Window only methods, but it were recently been added to DedicatedWorkerGlobalScope (well both really) via the mixin:

interface mixin AnimationFrameProvider {
  unsigned long requestAnimationFrame(FrameRequestCallback callback);
  undefined cancelAnimationFrame(unsigned long handle);
};

Other cases I've seen document this in the mixin and then refer to it in Window say as "methods documented in other classes". What is it you see happening here - it gets documented in both places?

For reference: #1519

@jpmedley
Copy link
Collaborator

jpmedley commented Feb 1, 2021

In the short term, yes. (I know. 😞). In the long term we need some kind of inclusion mechanism. This is the only way to keep information out of MDN that has no relevance to writing an actual script.

@hamishwillee
Copy link
Collaborator

Thanks @jpmedley. We do "kind of" have page inclusion now via a macro - that might be able to pull specific sections for methods from mixin docs into relevant pages? That mechanism feels fragile to me though.

@rachelandrew
Copy link
Collaborator

It would be lovely for a mixin to also be able to be a mixin in the docs :)

For now it would just be good to have a way forward that works for the majority of cases and that doesn't close the door on being able to address it in a better way in future. At least if we know where all the examples are, and they have been documented in a consistent way that should make it easier to convert them to some kind of inclusion mechanism in future.

I will highlights once I come across as I'm working. My open PRs for TextEncoderStream and TextDecoderStream both reference properties included via mixin.

For example: https://encoding.spec.whatwg.org/#interface-textdecoderstream includes TextDecoderCommon and GenericTransformStream. In this case I have documented these as if they were regular properties which is as option 2.

@rachelandrew
Copy link
Collaborator

So ... if I am adding ARIAMixin, which was AriaAttributes to Element, for BCD does that mean adding all of these to the 9000+ line long Element BCD file?

@Elchi3
Copy link
Member Author

Elchi3 commented Feb 2, 2021

So ... if I am adding ARIAMixin, which was AriaAttributes to Element, for BCD does that mean adding all of these to the 9000+ line long Element BCD file?

I've proposed a solution for BCD in mdn/browser-compat-data#8929

@Elchi3
Copy link
Member Author

Elchi3 commented Feb 3, 2021

I've made a proof of concept using HTMLHyperlinkElementUtils: #2046

@chrisdavidmills
Copy link
Contributor

I really like this proposal. I think it does what we've needed to do for a long time now. I also agree with the BCD approach outlined in mdn/browser-compat-data#8929. Having a separate file for the stuff defined in a mixin really does help make sense of it all in terms of the compat data.

For quite some time I've thought that we should define mixins in separate page sets, and then have some kind of clever way of slurping them into the actual pages we want readers to see, e.g. have a separate set of https://developer.mozilla.org/en-US/docs/Web/API/HTMLHyperlinkElementUtils pages, but then in the final build render separate https://developer.mozilla.org/en-US/docs/Web/API/HTMLAnchorElement and https://developer.mozilla.org/en-US/docs/Web/API/HTMLAreaElement pages based on them.

However, as I think about the implementation of this more, I wonder if that would be worth the effort. You would still need separate descriptions, syntax, examples, BCD, etc. for the two different page sets, if you were doing it properly.

I am going to go and review some of @Elchi3 's proof of concepts now, to see if any issues present themselves. If not, I'd say let's write this up in the MDN meta docs, and move forward with this way of doing things.

@foolip
Copy link
Contributor

foolip commented Feb 10, 2021

I'm very happy to see mixins being phased out, but want to quote what I said about WindowOrWorkerGlobalScope in the BCD issue:

We could naively flatten these mixins and end up with api.Window.fetch + api.WorkerGlobalScope.fetch instead of api.WindowOrWorkerGlobalScope.fetch. However, I think we should treat these mixins as different from all the rest.

It's an accident of how Web IDL works that the exposure set of interfaces like Request is defined using [Exposed=(Window,Worker)] while the exposure set of methods like fetch is defined using partial interface mixin WindowOrWorkerGlobalScope. In MDN documentation and in BCD, I think we should find a common way to represent the worker exposure. Many different approaches could work, but my first suggestion would be api.WindowOrWorkerGlobalScope.fetchapi.fetch and working through the many consequences of that.

However, I think it would be smart to set WindowOrWorkerGlobalScope aside in the mixin flattening, and sort it out in a later step, because the issues are very different.

@Elchi3
Copy link
Member Author

Elchi3 commented Feb 10, 2021

Thanks @foolip, I agree to leave this out for now and worry about it later. I also assume there will be exceptions to our new mixin documentation convention because the Web is not defined very consistently :)

@Elchi3
Copy link
Member Author

Elchi3 commented Feb 15, 2021

I've sent #2366 to update mixins in the MDN contribution docs according to the discussion here.

To-Do list for existing docs that need updates/refactoring/moving/deletion:

Unfortunately, I run into issues when moving pages around, so it is a bit slow going and hairy on the MDN side of things. We made some progress on the BCD side already, though. See mdn/browser-compat-data#8929

Also, for mixins that exists for workers, it might be a bit more complicated and we're still figuring things out. However, for mixins that don't touch on the worker topic, docs and compat data are vastly improved thanks to this effort.

@Elchi3 Elchi3 self-assigned this Feb 16, 2021
@jpmedley
Copy link
Collaborator

Many different approaches could work, but my first suggestion would be api.WindowOrWorkerGlobalScope.fetch → api.fetch and working through the many consequences of that.

That would resolve to a path like /api/fetch. Since I can call fetch() without reference to a global or instantiated object, that would work. But is that generally true of everything with WindowOrWorkerGlobalScope? I don't want to replace one thing that looks like a real code reference but isn't with another thing that looks like a real code reference but isn't.

@foolip
Copy link
Contributor

foolip commented Feb 17, 2021

Many different approaches could work, but my first suggestion would be api.WindowOrWorkerGlobalScope.fetch → api.fetch and working through the many consequences of that.

That would resolve to a path like /api/fetch. Since I can call fetch() without reference to a global or instantiated object, that would work. But is that generally true of everything with WindowOrWorkerGlobalScope? I don't want to replace one thing that looks like a real code reference but isn't with another thing that looks like a real code reference but isn't.

This is the right question to ask! For all of the methods on WindowOrWorkerGlobalScope, I believe invoking them directly as fetch() rather than self.fetch() is idiomatic. They are: atob(), btoa(), clearInterval(), clearTimeout(), createImageBitmap(), fetch(), queueMicrotask(), setInterval(), setTimeout().

For attributes, while they can be accessed directly, I think for at least these it's more idiomatic to access them via the global object, because it makes it clear they're not local variables: self.crossOriginIsolated, self.isSecureContext, self.origin.

Then there's caches and indexedDB, where I suspect there's more variation in style. I'd certainly write indexedDB.open(...), but the example uses window.indexedDB.open('toDoList').

On the URL, another option would be /api/Global_Objects/fetch, more analogous to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt

I think that looking at the members of Window and WorkerGlobalScope will reveal more tricky cases. I think especially a method like window.resizeTo(), while it can be invoked as just resizeTo(), would not be great to give the same treatment as fetch().

@jpmedley
Copy link
Collaborator

That sounds like problems with how we need to explain them on actual pages. I've created an issue capture this.

FWIW, I think it would be a good idea to move these under Global_Obects.

@Rumyra Rumyra added needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. MDN:Project Anything related to larger core projects on MDN and removed needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. labels Jun 7, 2021
@sideshowbarker
Copy link
Collaborator

I propose we move this to the Discussions tracker.

@Elchi3 Elchi3 closed this as completed Jun 9, 2021
@mdn mdn locked and limited conversation to collaborators Jun 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
MDN:Project Anything related to larger core projects on MDN
Projects
None yet
Development

No branches or pull requests

10 participants