Add docs npm script to adblockpluschrome

[erikvold]

As part of the effort to reduce the dependency on buildtools it would be useful to convert the docs to be generated via npm by configuring package.json accordingly resulting in a user level change from ./build.py docs docs to npm run docs.

Changes

add a script called "docs" to package.json and the jsdoc dependency.

Note

Doing this would be most simple if we have the output of jsdoc go into "docs" in the root directory of the repository.

[jsonesen]

Manish, Winsley, Thomas,

Is this something you all are okay with? If so I have patch sets prepared. This will be useful for us to not depend on buildtools to generate docs.

[sebastian]

For reference, this is part of a larger effort to phase out (the Python-based) buildtools. The changes itself look good to me, the next step would be integrating this with GitLab CI/CD.

[mjethani]

Makes sense to me.

[jsonesen]

I just noticed that abpui is pretty baren from a docstring standpoint, should it still be added there? Definitely makes sense for abpchrome and abpcore

[abpbot]

A commit referencing this issue has landed:
​Issue 7103 - Add jsdoc to package.json

[greiner]

Replying to jsonesen:

I just noticed that abpui is pretty baren from a docstring standpoint, should it still be added there? Definitely makes sense for abpchrome and abpcore

Feel free to create a ticket for it, if you think it'd be useful.

[sebastian]

This issue is about getting rid of the buildtools dependency for generating the docs. We currently don't provide any API documentation for adblockplusui (not through buildtools anyway).

Whether to provide API documentation for adblockplusui in the future is unrelated of this issue.
FWIW, I don't think that this would be too useful anyway, as (unlike for adblockpluscore and adblockpluschrome) there are no consumers on the API-level of the user interface.

[mjethani]

Just want to add that it would be really nice if after every commit the latest version of the documentation was available on some website. I don't know if this has already been set up. Anyway, it's unrelated to the issue, but I just wanted to mention it.

[sebastian]

We ​have that already. However, currently sitescripts is calling build.py docs. This issue is about doing that through an npm script instead, and eventually running it from GitLab CI.

[tlucas]

FWIW, we might be able to use gitlab-pages for that purpose, after we landed this.

[kzar]

So far this is a Core issue, but with a change landed to Platform (adblockpluschrome) and therefore the release milestone set...

Jon, please could you change this to be a Platform issue, then file a new issue for the Core change, and update your review to use that issue number?

[kzar]

The ​change already pushed to adblockpluschrome doesn't seem to work. I get an error about a missing jsdoc.conf file. Your review for the similar adblockpluscore change adds that file, did you forget to stage it here?

[kzar]

Also, this issue hasn't been triaged yet, and doesn't detail how it's supposed to work. For example, your change attempts to put the docs in adblockpluschrome/docs, where as the existing script you mention puts them in adblockpluschrome/buildtools/docs. Which is correct?

[erikvold]

Replying to kzar:

For example, your change attempts to put the docs in adblockpluschrome/docs, where as the existing script you mention puts them in adblockpluschrome/buildtools/docs. Which is correct?

Yes the idea is to get rid of the buildtools dir.

[kzar]

Well, then please can you update the issue description to mention that, and any other relevant details?

[jsonesen]

Well, then please can you update the issue description to mention that, and any other relevant details?

Absolutely sorry, I see now it is very sparse.

We ​have that already. However, currently sitescripts is calling build.py docs. This issue is about doing that through an npm script instead, and eventually running it from GitLab CI.

I think he means it would be nice to also have for the next branch which I agree (unless I am mistaken and that can be found somewhere)

The ​change already pushed to adblockpluschrome doesn't seem to work. I get an error about a missing jsdoc.conf file. Your review for the similar adblockpluscore change adds that file, did you forget to stage it here?

Yikes, my bad that was a big mistake. Also, thinking about this makes me think I should have it output to ./buildtools/docs until that is all worked out.

[sebastian]

Replying to kzar:

I get an error about a missing jsdoc.conf file. Your review for the similar adblockpluscore change adds that file, did you forget to stage it here?

Yeah, I see the file jsdoc.conf in the review but not in the commit. Any reason we didn't add the missing file in a follow-up commit yet?

Replying to jsonesen:

Also, thinking about this makes me think I should have it output to ./buildtools/docs until that is all worked out.

The whole point of this change is to move away from buildtools. So why should the generated docs still go into buildtools/docs? It seems putting them into docs (under the top-level) as the current patch does is exactly what we want, or do I miss something?

[abpbot]

A commit referencing this issue has landed:
​Issue 7103 - Add missing jsdoc.conf

[sebastian]

The next step is to integrate those changes with GitLab CI, running npm run docs on every commit in master and uploading the docs to the ​website.

Tristan, can you help with this or provide some guidance?

[tlucas]

Replying to sebastian:

The next step is to integrate those changes with GitLab CI, running npm run docs on every commit in master and uploading the docs to the ​website.

Tristan, can you help with this or provide some guidance?

Yes, i'd be happy to help with this, in case this is not too urgent, i'd tackle this after my bigger release-project.

[jsonesen]

Manish raised a good point during the review for the addition of jsdocs to core, the configuration file should probably be hidden since this is the convention used for other configurations (with the exception of package.json) what do you think?

[sebastian]

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the ​new issue tracker.