use sphinx-js to integrate jsdocs into `./mach doc` output

RESOLVED FIXED

Status

RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: rhelmer, Assigned: rhelmer)

Tracking

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
Bug 1352316 is adding jsdoc generation to mozilla-central, but these docs are separate from the sphinx output that `./mach doc` produces, which needs to be hosted and linked to from the sphinx docs.

It's also pretty clumsy to put things like code samples and lots of prose in the jsdoc comments themselves, but should still live alongside the code and not on a wiki.

Filing this bug to track getting sphinx-js working for mozilla-central. Once we can show that it's possible and have a fully-working example, we can propose switching over.
(Assignee)

Comment 1

a year ago
Matt has some concerns about using sphinx-js vs. raw jsdoc in bug 1352316 comment 13:

> PROS of jsdoc directly with TC:
> * Supports all features of jsdoc

sphinx-js uses jsdoc "explain" output so it has access to all jsdoc features. I think it generally shouldn't need any changes for new jsdoc features, but part of sphinx-js's job is to stay on top of that.

> * Will be kept up-to-date with new jsdoc features without work on our part
> * Doesn't require adding a line to an RST file for each method, any documented method will appear in the docs automatically

I think implementing `automodule` in sphinx-js for both JS modules (JSM primarily but there are some other sorts of modules around too) would solve this... part of opting in to having docs generated+published would be to (at minimum) create a .rst file with nothing `automodule`.

> * Built immediately as part of m-c instead of requiring someone to update a separate GH repo

Ideally we'd just have a job that runs `./mach doc` once and then hosts it all in one place. This could be a taskcluster job, like the one bug 1352316 uses.

> * Doesn't require learning anything about sphinx/rst

As above I think the bare minimum would be pretty darn minimal.

We do have active docs in the tree (I know Telemetry uses it actively), and we need some way to document outside of jsdoc comments... the only options that feel tenable to me are either markdown or rst, and the number of sphinx plugins and the fact we already use it makes the latter seem reasonable.


> CONS:
> * Not as well integrated into the RTD hierarchy. Links to JSDocs can be added from rst files though.
> ** Personally I see this as a reason not to use RTD… unfortunately we didn't set up a custom domain on RTD so now people link to the subdomain making it harder to move away :(

I'd be happy for us to stand up a new mozilla.org subdomain and deprecate everything else (MDN, gecko.rtd, etc linking appropriately to the new location).

We use "gecko.readthedocs.io" now and "gecko.developer.mozilla.org" was proposed - I am OK with that but I wonder if using "gecko" is going to age well, what with the Quantum work :) "firefox-dev" or just "firefox" as a subdomain might fare a little better.
(Assignee)

Comment 2

a year ago
(In reply to Robert Helmer [:rhelmer] from comment #1)
> We use "gecko.readthedocs.io" now and "gecko.developer.mozilla.org" was
> proposed - I am OK with that but I wonder if using "gecko" is going to age
> well, what with the Quantum work :) "firefox-dev" or just "firefox" as a
> subdomain might fare a little better.

This isn't very clear, sorry. I am proposing something like:

firefox.developer.mozilla.org

Or if we don't want to/can't use developer.mozilla.org for some reason something like:

firefox-dev.mozilla.org
Good news: just the other day we hooked up TaskCluster automation to upload the Sphinx docs to an S3 bucket (bug 1382729). That now happens any time a docs change lands on mozilla-central. The docs can be accessed at http://gecko-docs.mozilla.org.s3.amazonaws.com/index.html.

What we haven't done yet is hook up a proper hostname. I was planning to get that ball rolling. If I hadn't been distracted by this VCS security bug fire drill, I may have had something already. But if you want to take over the process, go for it!

I agree that "gecko" isn't appropriate for what the docs are. I can't recall why I didn't use "firefox" on RTD. Either someone squatted on it already or I didn't want to steal the name for what at the time was mainly Python API docs and not very Firefox centric. I'd encourage you to think big this time around and squat on "firefox" as the main name. We can restructure the docs accordingly to reflect an ambitious and widely-scoped name.

All signs seem to point to us hitting the scaling limits of RTD. So my intention is to end of life gecko.rtd and have everything point to something that TaskCluster generates. That new S3 bucket likely fits the bill. Using TaskCluster means we have full control over the docs "build" environment. So go to town with advanced features like sphinx-js :)

Finally, we had a little thread about builds docs a few months back that you may enjoy reading: https://groups.google.com/d/msg/mozilla.dev.builds/cp4bJ1QJXTE/Xqy_nHV5DAAJ. I owe a reply there. But I think all the signs are pointing towards a unified, Sphinx-based, in-tree docs solution. It isn't as user-friendly as editing a wiki. But if we implement some of the advanced workflow stuff I've been conjuring up, it likely will be :)
Depends on: 1382729
When you agree on a domain, I can help with picking new names for buckets and setting up a cloudfront endpoint for it (that's the cheapest way to get SSL for a custom domain backed by S3).  Currently those are all owned by the Taskcluster AWS account, so if there's an account that makes more sense this would be a good time to use it.  I don't expect it to cost a lot, so it aside from "making sense" it doesn't matter which account is used.

As a matter of personal opinion, I find that wiki editing tends toward people with limited understanding (who just *read* the existing wiki content) writing down what they think they understand, and wikis tend to be out of date. I've given up looking at MDN for development-related information.  On the other hand, the in-tree documentation tends to be updated by the person making the changes and is easily tied to the source (either via jsdoc stuff or, as in taskgraph, python tests checking that the docs and the source align) so it doesn't get out of date.
Not to mention that in-tree docs get the full use of branches, so you don't have to wait till the very end and try to time your wiki updates.

FWIW, RTD supports sphinx-js now, but I agree it's better to control our own doc-building, especially if we already have 90% of the infra set up.
To clarify my comments on RTD, for *years* gecko.rtd has built intermittently. I've had to poke them a few times to unbreak thing. In the past few months, gecko.rtd has been failing with a spectacularly high rate. I think it even contributed to at least one group creating their own Sphinx docs out-of-tree because it was so unreliable. We gave RTD a shot and it worked for several years. Now it's time to build and host the docs ourselves: that will be less pain and cost in the long run.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8901007 - Flags: feedback?(erik)
(Assignee)

Updated

a year ago
Attachment #8901008 - Flags: feedback?(erik)

Comment 9

a year ago
mozreview-review
Comment on attachment 8901007 [details]
Bug 1389341 - add basic support for sphinx-js to mach doc

https://reviewboard.mozilla.org/r/172472/#review177734

Seems pretty straightforward!
Attachment #8901007 - Flags: review?(gps) → review+

Comment 10

a year ago
mozreview-review
Comment on attachment 8901008 [details]
Bug 1389341 - start generating jsdoc for AddonManager API

https://reviewboard.mozilla.org/r/172474/#review177736

I'll hold off granting review of this until the whole series is ready.

::: tools/docs/conf.py:44
(Diff revision 1)
>      'sphinx_js',
>  ]
>  
>  # JSDoc must run successfully for dirs specified, so running
>  # tree-wide (the default) will not work currently.
> -js_source_path = ''
> +js_source_path = 'toolkit/mozapps/extensions'

This part seems a bit wonky. How are we going to extend this to the rest of the repo?

For Python, we have a ``SPHINX_PYTHON_PACKAGE_DIRS`` moz.build variable. We mave some aggregate all these directories in ``tools/docs/moztreedocs/__init__.py`` into a central "staging" directory. This is necessary to make Sphinx happy, as Sphinx seems to insist that certain docs have a certain hierarchy. Perhaps we need to do something similar for JS files?

Also, is there a way to get it to generate docs for all the JS in the tree? We kinda have that in place for Python today.
Attachment #8901008 - Flags: review-

Comment 11

a year ago
mozreview-review
Comment on attachment 8901007 [details]
Bug 1389341 - add basic support for sphinx-js to mach doc

https://reviewboard.mozilla.org/r/172472/#review177738

::: tools/docs/mach_commands.py:40
(Diff revision 1)
>      @CommandArgument('--http', const=':6666', metavar='ADDRESS', nargs='?',
>          help='Serve documentation on an HTTP server, e.g. ":6666".')
>      def build_docs(self, what=None, format=None, outdir=None, auto_open=True, http=None):
>          self._activate_virtualenv()
> -        self.virtualenv_manager.install_pip_package('sphinx_rtd_theme==0.1.6')
> +        self.virtualenv_manager.install_pip_package('sphinx_rtd_theme==0.2.4')
> +        self.virtualenv_manager.install_pip_package('sphinx-js==2.0.1')

Will this introduce a node.js requirement to build the docs? If so, you may need to update the Docker image to install the appropriate dependencies. A Try run should tell all.
(Assignee)

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8901008 [details]
Bug 1389341 - start generating jsdoc for AddonManager API

https://reviewboard.mozilla.org/r/172474/#review177736

> This part seems a bit wonky. How are we going to extend this to the rest of the repo?
> 
> For Python, we have a ``SPHINX_PYTHON_PACKAGE_DIRS`` moz.build variable. We mave some aggregate all these directories in ``tools/docs/moztreedocs/__init__.py`` into a central "staging" directory. This is necessary to make Sphinx happy, as Sphinx seems to insist that certain docs have a certain hierarchy. Perhaps we need to do something similar for JS files?
> 
> Also, is there a way to get it to generate docs for all the JS in the tree? We kinda have that in place for Python today.

I'm actually not sure if it supports multiple paths or not.. maybe Erik knows? If not, I think that's something we'll need to add to sphinx_js. Moving this out to a moz.build variable sounds like a fine idea. I don't think there's any reason we'd have to build into a staging directory first, I think sphinx-js takes care of representing the JSDoc output in a way that Sphinx requires.

I believe that the default is for jsdoc to run recursively over the whole tree, if `js_source_path` is not specified. Most of the JSDoc annotations we have in the tree aren't parseable by modern JSDoc though and will cause the run to blow up, fortunately there's an ESLint rule we can enable as we get things up to snuff which should catch problems during the lint pass.

Comment 13

a year ago
mozreview-review
Comment on attachment 8901008 [details]
Bug 1389341 - start generating jsdoc for AddonManager API

https://reviewboard.mozilla.org/r/172474/#review177744

r=me for the AOM changes. It could still use more work, but it's a good start.

I'll defer to gps on the rest.

::: toolkit/mozapps/extensions/AddonManager.jsm:937
(Diff revision 1)
> -   * @param  aProvider
> -   *         The provider to register
> +   * @param {string} aProvider -The provider to register
> +   * @param {array} aTypes -  An optional array of add-on types

Nit: Weird spacing around hyphens.

::: toolkit/mozapps/extensions/AddonManager.jsm:938
(Diff revision 1)
>  
>    /**
>     * Registers a new AddonProvider.
>     *
> -   * @param  aProvider
> -   *         The provider to register
> +   * @param {string} aProvider -The provider to register
> +   * @param {array} aTypes -  An optional array of add-on types

Nit: `{Array<string>}` or `{string[]}`, and optional types should have their names in square brackets (`[aTypes]`)

::: toolkit/mozapps/extensions/AddonManager.jsm:2262
(Diff revision 1)
>     * Installs a temporary add-on from a local file or directory.
> +   *
>     * @param  aFile
>     *         An nsIFile for the file or directory of the add-on to be
>     *         temporarily installed.
>     * @return a Promise that rejects if the add-on is not a valid restartless

Seems like your ESLint prefer rule should throw for this.

::: toolkit/mozapps/extensions/AddonManager.jsm:2394
(Diff revision 1)
> -   * @resolves The found Addon or null if no such add-on exists.
> -   * @rejects  Never
> +   * resolves The found Addon or null if no such add-on exists.
> +   * rejects  Never

Maybe make these complete sentences, and indent them so it's clear they're part of the returns description?

::: toolkit/mozapps/extensions/AddonManager.jsm:2417
(Diff revision 1)
>    },
>  
>    /**
>     * Asynchronously get an add-on with a specific Sync GUID.
>     *
> +   *

Seems unnecessary

::: toolkit/mozapps/extensions/AddonManager.jsm:2594
(Diff revision 1)
>    },
>  
>    /**
>     * Removes an AddonManagerListener if the listener is registered.
>     *
> -   * @param  aListener
> +   * @param {object} aListener

The type should probably be `AddonManagerListener` for these.

::: toolkit/mozapps/extensions/AddonManager.jsm:2608
(Diff revision 1)
>    },
>  
>    /**
>     * Adds a new AddonListener if the listener is not already registered.
>     *
> -   * @param  aListener
> +   * @param {object} aListener

The type should probably be `AddonListener` for these.

::: toolkit/mozapps/extensions/AddonManager.jsm:2636
(Diff revision 1)
>    },
>  
>    /**
>     * Adds a new TypeListener if the listener is not already registered.
>     *
> -   * @param  aListener
> +   * @param {object} aListener

The type should probably be `TypeListener` for these.

::: toolkit/mozapps/extensions/AddonManager.jsm:3541
(Diff revision 1)
>  
>    get __AddonManagerInternal__() {
>      return AppConstants.DEBUG ? AddonManagerInternal : undefined;
>    },
>  
> +  /** Boolean indicating whether AddonManager startup has completed. */

Does this work as expected? Usually properties need something like `@property {type} propertyName ...`

::: toolkit/mozapps/extensions/ChromeManifestParser.jsm:45
(Diff revision 1)
>     * secondary manifests it references.
>     *
>     * @param  aURI
>     *         A nsIURI pointing to a chrome manifest.
>     *         Typically a file: or jar: URI.
> -   * @return Array of objects describing each manifest instruction, in the form:
> +   * @return {Array} -  objects describing each manifest instruction, in the form:

Nit: `{object[]}` or `{Array<object>}`
Attachment #8901008 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a year ago
mozreview-review-reply
Comment on attachment 8901008 [details]
Bug 1389341 - start generating jsdoc for AddonManager API

https://reviewboard.mozilla.org/r/172474/#review177744

> Seems like your ESLint prefer rule should throw for this.

Indeed... I'll run it locally before I push this to make sure I didn't miss anything.

> Does this work as expected? Usually properties need something like `@property {type} propertyName ...`

IIRC jsdoc understands getters without needing further annotation, will double-check before landing.
(Assignee)

Comment 16

a year ago
Hm, so apparently I hadn't resolved all the eslint jsdoc failures for toolkit/mozapps/extensions/ in this patch... will do that today. I have verified that the `mach doc`-generated HTML is what I'd expect from a quick look though.

Comment 17

a year ago
mozreview-review
Comment on attachment 8901008 [details]
Bug 1389341 - start generating jsdoc for AddonManager API

https://reviewboard.mozilla.org/r/172474/#review178090

Mostly I just want to make sure you meant to capitalize those few spots as you did. I've never seen it that way before. Otherwise, looks great!

::: toolkit/mozapps/extensions/AddonManager.jsm:937
(Diff revision 1)
> -   * @param  aProvider
> -   *         The provider to register
> +   * @param {string} aProvider -The provider to register
> +   * @param {array} aTypes -  An optional array of add-on types

FWIW, the hyphens are totally optional

::: toolkit/mozapps/extensions/AddonManager.jsm:938
(Diff revision 1)
>  
>    /**
>     * Registers a new AddonProvider.
>     *
> -   * @param  aProvider
> -   *         The provider to register
> +   * @param {string} aProvider -The provider to register
> +   * @param {array} aTypes -  An optional array of add-on types

Isn't the usual capitalization "Array"? Btw, did you know you can use Haskell-style notation so it's easy to express what it's an array of, like `number[]`? http://usejsdoc.org/tags-type.html

::: toolkit/mozapps/extensions/AddonManager.jsm:2608
(Diff revision 1)
>    },
>  
>    /**
>     * Adds a new AddonListener if the listener is not already registered.
>     *
> -   * @param  aListener
> +   * @param {object} aListener

Again, caps that are different from JS usage and the jsdoc docs: "Object"

::: toolkit/mozapps/extensions/docs/AddonManager.rst:2
(Diff revision 1)
> +AddonManager Reference
> +=======================

1 char longer than necessary ;-)

::: tools/docs/conf.py:44
(Diff revision 1)
>      'sphinx_js',
>  ]
>  
>  # JSDoc must run successfully for dirs specified, so running
>  # tree-wide (the default) will not work currently.
> -js_source_path = ''
> +js_source_path = 'toolkit/mozapps/extensions'

sphinx-js doesn't support multiple source paths right now, though I could fix that in a few minutes. I think it's a decent idea, and it wouldn't interfere with later efforts to make it recurse by default (https://github.com/erikrose/sphinx-js/issues/16). There's certainly value in a large project like this one of being able to clean up one subtree at a time and not have to pay the runtime of scanning the whole project.

The 2 choices are (1) have sphinx-js support multiple source paths or (2) point sphinx-js to the root, and turn recursion on in your jsdoc.json. Confirm that you think (1) is better, and I'll go make it happen. :-) [Ed: I see you said that jsdoc blows up if it can't parse things. So I'll just assume (1).]
Attachment #8901008 - Flags: review-
(Assignee)

Comment 18

a year ago
mozreview-review-reply
Comment on attachment 8901008 [details]
Bug 1389341 - start generating jsdoc for AddonManager API

https://reviewboard.mozilla.org/r/172474/#review178090

> Isn't the usual capitalization "Array"? Btw, did you know you can use Haskell-style notation so it's easy to express what it's an array of, like `number[]`? http://usejsdoc.org/tags-type.html

Yep kmag pointed this out - I really like this notation! It's `{string[]} [aTypes]` in the latest patch.

> Again, caps that are different from JS usage and the jsdoc docs: "Object"

We prefer lower-case (enforced by eslint) for other built-ins like `{boolean}` and `{number}` and so on... looks like we don't enforce it for object but I prefer the lower-case FWIW :)

> sphinx-js doesn't support multiple source paths right now, though I could fix that in a few minutes. I think it's a decent idea, and it wouldn't interfere with later efforts to make it recurse by default (https://github.com/erikrose/sphinx-js/issues/16). There's certainly value in a large project like this one of being able to clean up one subtree at a time and not have to pay the runtime of scanning the whole project.
> 
> The 2 choices are (1) have sphinx-js support multiple source paths or (2) point sphinx-js to the root, and turn recursion on in your jsdoc.json. Confirm that you think (1) is better, and I'll go make it happen. :-) [Ed: I see you said that jsdoc blows up if it can't parse things. So I'll just assume (1).]

Yes please do (1), thanks!
Comment hidden (mozreview-request)
(Assignee)

Comment 20

a year ago
OK so there's some more broken JSDoc in the addons manager code I need to fix, to get eslint passing.

Once that's done, we still need to:

1) have sphinx-js support multiple code paths per comment 17 (might as well use a moz.build variable too) and pin the appropriate version
2) get jsdoc installing into the docker container per comment 11
(Assignee)

Comment 21

a year ago
Getting the jsdoc eslint to pass right now is going to be a massive patch - I think it's worth doing but I'd like to get something more minimal going soon that we can build off of, so I am going to leave that out of this patch and do it in a separate bug.
Comment hidden (mozreview-request)
(Assignee)

Comment 23

a year ago
I'm pretty sure I've narrowed down why we can't have nice output for the "public class forwards to private class" style of encapsulation the AddonManager impl uses, there's a test case for this and also to make sure we handle object initializer style well in https://github.com/erikrose/sphinx-js/pull/15 that I've just revived since I think I see the underlying sphinx-js issue now.

For instance this comes up with AddonManager, which has public methods that simply call methods of the same name on AddonManagerInternal (the latter of which contains the actual JSDoc we want to expose)

I get the JSDoc HTML output I want if I use the jsdoc "@lends AddonManager" on AddonManagerInternal, and @private on the AddonManager methods that I want to suppress (the only comments there are things like "see the real implementation elsewhere")
(Assignee)

Comment 24

a year ago
All that said, I think we should probably just rewrite AddonManager to be a much simpler class post-57, and depend on some other way of information hiding (maybe even just advisory like the jsdoc @private annotation).

Still, it's a thing that exists and we should support it, if only to make moving existing code easier.
I just released sphinx-js 2.1 (https://pypi.python.org/pypi/sphinx-js/), which adds support for multiple source folders (so we can migrate moz-central to valid jsdoc one folder at a time) and tolerates unusual JS file extensions (like .jsm). https://github.com/erikrose/sphinx-js/pull/15 turned out not to actually be a blocking issue--or maybe an issue at all (see https://github.com/erikrose/sphinx-js/pull/15#issuecomment-325771532). We should be all set on the sphinx-js side now.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

a year ago
mozreview-review
Comment on attachment 8901008 [details]
Bug 1389341 - start generating jsdoc for AddonManager API

https://reviewboard.mozilla.org/r/172474/#review180038

Otherwise, lgtm!
Attachment #8901008 - Flags: review+

Comment 29

a year ago
mozreview-review-reply
Comment on attachment 8901008 [details]
Bug 1389341 - start generating jsdoc for AddonManager API

https://reviewboard.mozilla.org/r/172474/#review178090

> Yes please do (1), thanks!

One small hiccough: atm, relative paths to JS constructs (e.g. ./some/folder/someFile.thing) are construed relative to the js_source_path. So I have to figure out what the new semantics should be. I may add a js_relative_path_root or something.
(Sorry, late-published an old review comment.)
If bug 1395752 lands first, the patches here will need a minor update to ensure JS files are available to CI.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8901007 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8901008 - Attachment is obsolete: true
(Assignee)

Comment 34

a year ago
Had a lot of trouble rebasing this so ended up redoing and wiping out the previous reviews, sorry about that... 

In any case I realized bug 1352316 already got review and is just doing an `npm install -g jsdoc` so I did that here too.

Doing a try run now to see if it's actually doing the thing we want.
(Assignee)

Comment 35

a year ago
(In reply to Gregory Szorc [:gps] from comment #31)
> If bug 1395752 lands first, the patches here will need a minor update to
> ensure JS files are available to CI.

I couldn't figure out what needs to change from a quick read of that bug, would you mind specifying? Thanks!
Flags: needinfo?(gps)

Comment 36

a year ago
mozreview-review
Comment on attachment 8908335 [details]
Bug 1389341 - start generating jsdoc for AddonManager API

https://reviewboard.mozilla.org/r/179968/#review185172

::: toolkit/mozapps/extensions/AddonManager.jsm:2389
(Diff revision 1)
>    },
>  
>    /**
>     * Asynchronously gets an add-on with a specific ID.
>     *
> -   * @param  aID
> +   * @type {function}

Is this necessary?

::: toolkit/mozapps/extensions/AddonManager.jsm:2604
(Diff revision 1)
>    },
>  
>    /**
>     * Adds a new AddonListener if the listener is not already registered.
>     *
> -   * @param  aListener
> +   * @param {AddonManagerListener} aListener

`AddonListener`

::: toolkit/mozapps/extensions/AddonManager.jsm:2618
(Diff revision 1)
>    },
>  
>    /**
>     * Removes an AddonListener if the listener is registered.
>     *
> -   * @param  aListener
> +   * @param {object}  aListener

`AddonListener`
Attachment #8908335 - Flags: review?(kmaglione+bmo) → review+
(In reply to Robert Helmer [:rhelmer] from comment #35)
> (In reply to Gregory Szorc [:gps] from comment #31)
> > If bug 1395752 lands first, the patches here will need a minor update to
> > ensure JS files are available to CI.
> 
> I couldn't figure out what needs to change from a quick read of that bug,
> would you mind specifying? Thanks!

build/sparse-profiles/sphinx-docs needs to add JavaScript files that Sphinx will need to build the docs. I anticipate it looking something like:

glob:**/*.js
glob:**/*.jsm

The "files-changed" entries in taskcluster/ci/source-test/doc.yml will also need to be updated to tell CI that JS file changes need to trigger Sphinx generation. Otherwise Sphinx won't run in CI if only JS files change in the commit.

Yes, these patterns are somewhat redundant. We'll figure out a way to unify them someday. Sparse checkout is pretty new and I don't want to over-engineer things until we have a better idea how things will shake out.
Flags: needinfo?(gps)

Comment 38

a year ago
mozreview-review
Comment on attachment 8908334 [details]
Bug 1389341 - add basic support for sphinx-js to mach doc

https://reviewboard.mozilla.org/r/179966/#review185552

This needs the sparse-profiles file update that I detailed in the bug. And it is missing the jsdoc.json file.

Other than that, this looks good.
Attachment #8908334 - Flags: review?(gps) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 41

11 months ago
mozreview-review
Comment on attachment 8908334 [details]
Bug 1389341 - add basic support for sphinx-js to mach doc

https://reviewboard.mozilla.org/r/179966/#review206510

Looks good!
Attachment #8908334 - Flags: review?(gps) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

11 months ago
One thing that was pointed out to me today is that `mach doc` now errors out in a very non-obvious way if jsdoc is not present... chatted w/ gps in IRC and we decided to just detect the presence of jsdoc on the current path and bail if not present.

There are a few other node tools throughout the tree (such as eslint) but there doesn't seem to be one standard way to do this yet.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 47

11 months ago
mozreview-review
Comment on attachment 8908334 [details]
Bug 1389341 - add basic support for sphinx-js to mach doc

https://reviewboard.mozilla.org/r/179966/#review206592
(Assignee)

Comment 48

11 months ago
OK the change from comment 44 wfm locally, and it appears to work on infra too (I accidentally ran too many jobs and canceled easrly, but the "Doc" job finished OK:) https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cc27c6489d6ad7bfe048f0eced2738c19a0e931

Comment 49

11 months ago
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2acaf83b5395
add basic support for sphinx-js to mach doc r=gps
https://hg.mozilla.org/integration/autoland/rev/026e085e839c
start generating jsdoc for AddonManager API r=kmag

Comment 50

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2acaf83b5395
https://hg.mozilla.org/mozilla-central/rev/026e085e839c
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.