Skip to content
This repository has been archived by the owner on Sep 7, 2021. It is now read-only.

Fix ordering linting errors in JS docs #447

Closed
Elchi3 opened this issue May 20, 2020 · 12 comments
Closed

Fix ordering linting errors in JS docs #447

Elchi3 opened this issue May 20, 2020 · 12 comments
Assignees
Labels
js-linting All issues that concern linting of JS docs

Comments

@Elchi3
Copy link
Member

Elchi3 commented May 20, 2020

#438 has landed and is quite brutal to us in terms of new errors in JS docs. It seems that over 300 errors are now about wrong ordering.

@Elchi3 Elchi3 added the js-linting All issues that concern linting of JS docs label May 20, 2020
@Elchi3
Copy link
Member Author

Elchi3 commented May 20, 2020

From the call just now:

Errors are thrown quite verbosely (multiple per page), so this isn't as bad as I thought. Will is going to look into making it more silent.

Also, one thing to look into is Polyfill sections and whether we should add it as optional ingredient.

@wbamberg
Copy link

I've filed #448 to make the linter log only one error when sections are out of order.

@wbamberg
Copy link

wbamberg commented May 20, 2020

I have had a go at this and we are now at 175 errors, down from 379 this morning. Mostly this is relocating H2#Polyfill but there were a few other obvious changes too.

Some questions.

H2#Syntax for properties

Many of our property pages include an H2#Syntax, but this is not in the "javascript-property" recipe. For example: SharedArrayBuffer.byteLength. I think: these don't add any value, we were right to exclude them from the "javascript-property" recipe, and we should remove them from property pages.

The same applies to some namespace pages: such as arguments and class pages such as Generator.

One exception I've seen is String: I think here we should integrate H2#Syntax into H2#Description, like this: https://wiki.developer.mozilla.org/en-US/docs/User:wbamberg/new-string. I don't think this is perfect, but it looks like an improvement to me anyway.

Old compat notes

Several pages include some ancient compat notes under H2 headings:

I think we should delete these.

TypedArray

https://wiki.developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray

This is a nasty one! It contains an H2#Syntax section, which it should not have since it is tagged as a namespace. I think it should be OK to remove this H2#Syntax: it's actually quite misleading since it describes the "abstract form" of the constructor that's actually implemented in the concrete typed arrays like Int8Array etc. There is no constructor TypedArray().

It's not a class by our definition, because it doesn't have a constructor. So it's tagged as a namespace. But it isn't quite a namespace either because it has instance members, which we currently hide inside a TypedArray prototype section.

This can work, from the point of view of linting, because H2#TypedArray_prototype gets treated as a prose.* section. But in terms of semantics and structured content, it's wrong.

Maybe we should make data.constructor optional for classes, to allow for this kind of abstract class? Are there any other examples of this kind of thing?

Proxy

https://wiki.developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy

This page and its subpages need work.

Proxy is a javascript-class, so it should have a constructor and should not have H2#Syntax. It also lists instance methods like handler.setPrototypeOf(), but these aren't at all instance methods of Proxy, are they? As far as I can see they are methods that can be defined on the handler argument to the Proxy constructor.

Honestly I think we need to rework this piece of the documentation completely.

A smaller point here: this page fails the linter because it includes an H2#Licensing_note at the end, which links to a dead wiki. Can we remove this? If not maybe we could move it into the H2#Description.

Miscellaneous

@wbamberg
Copy link

I also filed #449 about making polyfill an ingredient.

@Elchi3
Copy link
Member Author

Elchi3 commented May 21, 2020

Thank you so much for digging into content linting, Will! I'm super happy about this discussion on content structures! All what you've found is great insight!


Mostly this is relocating H2#Polyfill but there were a few other obvious changes too.

I see Polyfills are now above Examples. Not sure Polyfills are more important than Examples, but I don't know if our section order is by importance or what the criteria is... We probably need to dig into this some more at a later point anyway, so thanks for fixing this for the moment and let's discuss in the issue you've filed and see how we could address this properly.


H2#Syntax for properties

I agree to everything you say about these. We agreed back when we first created JS recipes that syntax sections for properties make no sense. Also agree about the other cases and for String: thanks for taking the time to rewrite this!


Old compat notes

I think we should apply the BCD criteria for "irrelevant features" here as well. If these notes talk about things that date back more than two years ago (roughly older than Firefox 60 at this point), we should remove it.


TypedArray

Yeah, I've been going back on forth on this one. You are right, it is neither a class nor a namespace. It is an abstract class (or "mixin" if you will) and those have no constructors. So, we could eliminate the abstraction and create sub pages under all concrete classes like "Int8Array", but that doesn't seem very practical. In API docs, we will hit exactly this question again as we have documented abstract classes (or mixins) for convenience there, too.

Maybe we should make data.constructor optional for classes, to allow for this kind of abstract class? Are there any other examples of this kind of thing?

I'm hesitant about this. I wonder if we should rather invent a new recipe called "abstract class" that doesn't allow to have a constructor ingredient but is otherwise the same as a class recipe (or maybe we find additional ingredients for abstract classes, for example a section that mentions all the concrete classes like the TypedArray page currently has)


Proxy

Proxy is a javascript-class, so it should have a constructor and should not have H2#Syntax. It also lists instance methods like handler.setPrototypeOf(), but these aren't at all instance methods of Proxy, are they? As far as I can see they are methods that can be defined on the handler argument to the Proxy constructor.

Yes, this is done poorly right now and doesn't work very well with our structures. I guess you are right about re-doing this completely.

A smaller point here: this page fails the linter because it includes an H2#Licensing_note at the end, which links to a dead wiki. Can we remove this? If not maybe we could move it into the H2#Description.

Remove it. As far as I remember the very first version of the Proxy docs were quite close to the ECMAScript wiki, but the content has changed completely since then and the ECMAScript wiki is no more, so there is no reason for this anymore (IANAL, though).


Miscellaneous

Also happy to remove all of these. Just adds confusion and is not maintainable for us.

@wbamberg
Copy link

wbamberg commented May 21, 2020

Thanks for your responses @Elchi3 !

I've had another go at this.

Mostly this is relocating H2#Polyfill but there were a few other obvious changes too.

I see Polyfills are now above Examples. Not sure Polyfills are more important than Examples, but I don't know if our section order is by importance or what the criteria is... We probably need to dig into this some more at a later point anyway, so thanks for fixing this for the moment and let's discuss in the issue you've filed and see how we could address this properly.

Realistically, unless we make polyfill an explicit ingredient it has to go here, because we have to treat it as a prose.* section. I don't actually think we should make it an explicit ingredient yet because it's not clear to me what its structure would look like (we know we don't like the current representation, but haven't worked out what we do want).

I don't know if our section order is by importance or what the criteria is

It is not by importance. BCD is very important but is at the end. In general, IMO, the start and end are where the most universally important stuff should be, and that's where we have put the more-defined stuff. In the middle is where custom content (prose.*) gets to go.

H2#Syntax for properties

I agree to everything you say about these. We agreed back when we first created JS recipes that syntax sections for properties make no sense. Also agree about the other cases and for String: thanks for taking the time to rewrite this!

OK, this is done!

Old compat notes

I think we should apply the BCD criteria for "irrelevant features" here as well. If these notes talk about things that date back more than two years ago (roughly older than Firefox 60 at this point), we should remove it.

All gone!

Miscellaneous

Also happy to remove all of these. Just adds confusion and is not maintainable for us.

All gone!


There are now just three pages (AFAICT) generating this error:

Array

This fails because of the Array instances section. I don't think this is adding anything useful - it just seems like basic JS stuff and not particular to Array. Why talk about changing the prototype here, and not for String or Date or any other class?

So I would recommend deleting this. If we wanted to keep it perhaps it could live as an H3 in Description?

Proxy

I filed mdn/sprints#3268 for this. Happy to take it if you like. I don't know if it should be considered to block this work, I suppose technically yes. But it will take a little time.

TypedArray

TypedArray

Yeah, I've been going back on forth on this one. You are right, it is neither a class nor a namespace. It is an abstract class (or "mixin" if you will) and those have no constructors. So, we could eliminate the abstraction and create sub pages under all concrete classes like "Int8Array", but that doesn't seem very practical. In API docs, we will hit exactly this question again as we have documented abstract classes (or mixins) for convenience there, too.

It is hard to know what to do about TypedArray, yes. There seem to be no good options. I think duplicating all these functions across all the concrete classes would be awful, and yet going from say Int32Array to TypedArray.toLocaleString is jarring.

Perhaps we could say TypedArray is not a class or a namespace page. It's just a guide page that exists as a parent of the TypedArray method pages, and could give an overview of TypedArray and link to the concrete subclasses? A lot of the stuff in that page that's pretending that TypedArray is a normal class, is very confusing:

ECMAScript 2015 defines a TypedArray constructor that serves as the [[Prototype]] of all TypedArray constructors. This constructor is not directly exposed: there is no global %TypedArray% or TypedArray property. It is only directly accessible through Object.getPrototypeOf(Int8Array) and similar. All TypedArrays constructors inherit common properties from the %TypedArray% constructor function. Additionally, all typed array prototypes (TypedArray.prototype) have %TypedArray%.prototype as their [[Prototype]].

I'm not sure how useful that is.

Maybe we should make data.constructor optional for classes, to allow for this kind of abstract class? Are there any other examples of this kind of thing?

I'm hesitant about this. I wonder if we should rather invent a new recipe called "abstract class" that doesn't allow to have a constructor ingredient but is otherwise the same as a class recipe

I think I would prefer to make data.constructor optional in "javascript-class", than to define a whole new recipe for one page. Recipes are key meta-documentation for writers, and the more recipes, the more cognitive load we are putting on them. The drawback of course is that most (almost all) classes do need constructors, and by making it optional we can't lint for it. Again, no good options.

Or maybe, like we do for "Specifications", you have to have a "Constructor" section, but it is allowed to say something like: "This class is not directly instantiable." instead of containing a link to the constructor page?

(or maybe we find additional ingredients for abstract classes, for example a section that mentions all the concrete classes like the TypedArray page currently has)

I did find another class that doesn't have a (user-visible) constructor: Generator. And that class doesn't have any concrete classes to list AFAICT.

I agree that mixins seem similar, but I haven't really thought about them yet, so not sure how they would inform this. But perhaps if we can, and unlike Generator or TypedArray, we should try to avoid exposing them to users entirely. I mean TypedArray has explanatory power, because people can understand that all the methods of Int32Array are also available on Float32Array. So as a concept it's worth talking about. But I'm not sure I feel the same about WindowEventHandlers.

@Elchi3
Copy link
Member Author

Elchi3 commented May 22, 2020

I've had another go at this.

\o/

Array

This fails because of the Array instances section. I don't think this is adding anything useful - it just seems like basic JS stuff and not particular to Array. Why talk about changing the prototype here, and not for String or Date or any other class?

So I would recommend deleting this. If we wanted to keep it perhaps it could live as an H3 in Description?

It's probably there, because it is very common to extend arrays, but I agree, you can do this with any class, and there is no good reason to have it there. (I also think, given smooshgate etc., we shouldn't really advertise this)

Proxy

I filed mdn/sprints#3268 for this. Happy to take it if you like. I don't know if it should be considered to block this work, I suppose technically yes. But it will take a little time.

Thanks for splitting this out. If you feel like taking it, go for it, but otherwise lets see if there is time left at the end or if we should schedule it properly next sprint. No need to rush anything.

TypedArray

Or maybe, like we do for "Specifications", you have to have a "Constructor" section, but it is allowed to say something like: "This class is not directly instantiable." instead of containing a link to the constructor page?

I think this is interesting, because then you make clear that this is an abstract class. I think I would prefer this option. Mainly because I'm a little bit worried that by making constructor optional, it will be forgotten on pages where we should really have a constructor section (not sure if this is a valid worry given lack of experience with linting, though).

For Generator, it might also make sense to explicitly mention that a generator object can be returned from a generator function, but there is no Generator() constructor like there is a Function() constructor (which is inconsistent of JavaScript, but I think authors of the JS language regret adding the Function() constructor in the first place and didn't want to repeat this error for generators).

I agree that mixins seem similar, but I haven't really thought about them yet, so not sure how they would inform this. But perhaps if we can, and unlike Generator or TypedArray, we should try to avoid exposing them to users entirely. I mean TypedArray has explanatory power, because people can understand that all the methods of Int32Array are also available on Float32Array. So as a concept it's worth talking about. But I'm not sure I feel the same about WindowEventHandlers.

Interesting take on the mixin debate. I haven't thought about it from this angle, but I think you make a good point.

@wbamberg
Copy link

OK so before I write an update to the linter, let's ask @ddbeck .

Daniel, context: data.constructor is a mandatory ingredient for javascript-class pages, and to satisfy it a page must have a link to a constructor page. This is fine for almost all JS classes, but two don't have constructors:

  • TypedArray is a kind of abstract class: you can only instantiate its concrete subclasses (like Uint8Array).

  • Generator can't be directly instantiated at all: it's returned by a generator function.

We thought of three options for dealing with this:

  1. a new recipe, that's like javascript-class but omits data.constructor. I don't like this because I'd really like to have as few recipes as possible, and having a recipe for just 2 cases seems wrong.

  2. make data.constructor optional: but then we can't raise an error if a JS class that should really have it (i.e. almost all of them) omits it.

  3. keep it mandatory but have an "escape hatch" where an author can include, instead of a link, some prose starting "This object is not directly instantiable". This is analogous to what we do for "Specifications", where you can include a link or the "Not part of any standard" text.

I am happy enough with 3. What do you think?

@ddbeck
Copy link
Contributor

ddbeck commented May 26, 2020

@wbamberg I like option 3 a lot! 👍

@wbamberg
Copy link

wbamberg commented May 28, 2020

@Elchi3 , now we have an escape hatch for objects that are not instantiable (#461), I have had a go at updating TypedArray. I've made some quite big changes so would appreciate a look.

I have:

  • changed it from a namespace to a class
  • renamed "Methods" and "Properties" to "Static methods" and "Static properties"
  • replaced the "TypedArray prototype" section with "Instance methods" and "Instance properties" sections
  • removed "Syntax", and added "Constructor", that incorporates some of the same material.

Unfortunately this uncovers more errors, because the linter complains that:

  • "TypedArray.length" in "Static properties"
  • "TypedArray.prototype.constructor" in "Instance properties"

...are not links. Sigh. I'm very tempted to just delete these.

@Elchi3
Copy link
Member Author

Elchi3 commented May 29, 2020

Very well done, Will! I like this version of the page a lot better.

Sigh. I'm very tempted to just delete these.

I actually deleted these from other JS class pages as they don't any value (and in terms of length I think it just adds confusion). I went ahead and deleted these.

@wbamberg
Copy link

OK, we now have 0 linter errors about ordering!

Summary
Macro: jsxref                                        7039 warnings  jsxref                                                    
Macro: JSRef                                         632 warnings   jsref                                                     
Macro: JsSidebar                                     225 warnings   jssidebar                                                 
Macro: optional_inline                               185 warnings   optional_inline                                           
H2 heading: Polyfill was not in recipe               82 warnings    javascript-method/unknown-heading                         
Macro: js_property_attributes                        49 warnings    js_property_attributes                                    
Macro: PreviousNext                                  32 warnings    previousnext                                              
Macro: Glossary                                      28 warnings    glossary                                                  
H2 heading: Addition was not in recipe               26 warnings    javascript-language-feature/unknown-heading               
Macro: deprecated_header                             22 warnings    deprecated_header                                         
H2 heading: Tutorials was not in recipe              18 warnings    landing-page/unknown-heading                              
Macro: non-standard_header                           14 warnings    non-standard_header                                       
Macro: domxref                                       13 warnings    domxref                                                   
Macro: obsolete_header                               11 warnings    obsolete_header                                           
Macro: EmbedTest262ReportResultsTable                10 warnings    embedtest262reportresultstable                            
Expected h2#Examples for data.examples               8 errors       javascript-language-feature/data.examples/expected-heading
H2 heading: Syntax was not in recipe                 8 warnings     javascript-property/unknown-heading                       
Handler for data.link_lists is unimplemented         6 warnings     landing-page/data.link_lists/handler-not-implemented      
Expected h2#Syntax for prose.syntax                  4 errors       javascript-language-feature/prose.syntax/expected-heading 
Macro: ReadOnlyInline                                4 warnings     readonlyinline                                            
Expected h2#Examples for data.examples               4 errors       javascript-property/data.examples/expected-heading        
Macro: EmbedLiveSample                               3 warnings     embedlivesample                                           
H2 heading: Usage_recommendations was not in recipe  3 warnings     javascript-class/unknown-heading                          
Macro: Previous                                      2 warnings     previous                                                  
H2 heading: Properties was not in recipe             2 warnings     javascript-namespace/unknown-heading                      
Expected h2#Examples for data.examples               2 errors       javascript-namespace/data.examples/expected-heading       
Expected h2#Examples for data.examples               2 errors       javascript-class/data.examples/expected-heading           
Macro: anch                                          2 warnings     anch                                                      
Macro: Next                                          1 warning      next                                                      
Macro: ListSubPages                                  1 warning      listsubpages                                              
H2 heading: Shim was not in recipe                   1 warning      javascript-error/unknown-heading                          
Macro: interwiki                                     1 warning      interwiki                                                 
H2 heading: Compatibility_notes was not in recipe    1 warning      javascript-constructor/unknown-heading                    

33 message types (8441 messages, ✖ 20 errors, ⚠ 8421 warnings) in 858 files

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
js-linting All issues that concern linting of JS docs
Projects
None yet
Development

No branches or pull requests

3 participants