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

Added JSDoc #1782

Merged
merged 42 commits into from Jun 27, 2020
Merged

Added JSDoc #1782

merged 42 commits into from Jun 27, 2020

Conversation

RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Mar 1, 2019

This PR adds generated JSDoc documentation to Prism (#1777) using the Minami theme. The generated docs directory will be accessible from prismjs.com/docs.

This contains the doc of all highlight functions, insertBefore, extend, all hook functions, and Prism.Token. I used the docs from our extending page as a base and added a few sentences.
This includes complete type information for all documented function (except for hooks where the type of the env parameter depends on the hook).

Because JSDoc requires a README for the index.html, I used Prism's README. We can also make one specifically for the doc. (I just didn't know what to write.)

Interactive demo. (Will only be available during the time this PR is open.)

Right now, only prism-core is used to generate the doc. Other documented plugins and languages will need some adjustment before JSDoc can handle them. And there's also the question of whether these components should even be part of the doc.

Implementation

I used gulp-jsdoc3, so npx gulp will also re-generate the doc (this takes less time than minifying the language definitions, so the build process won't take too much longer).

Because of some ... features of JSDoc, I had to make some debatable adjustments to the doc generation process:

  1. JSDoc only adds the newly generated doc. It doesn't remove outdated files, i.e. if the file documented was deleted or renamed. So to fix this, gulp will first delete the whole doc directory.
  2. You cannot disable the generation time in the Minami theme, so I use a regex on the HTML files to remove the date.
  3. JSDoc also includes a few font files which our theme doesn't use, so those get removed.

Another rather unfortunate thing is that JSDoc doesn't use Prism to highlight source code.
We could make it with a few regex replacements but that would only add another point on my list of debatable decisions.


This resolves #1777.
This resolves #1555.
This resolves #1774.

@RunDevelopment
Copy link
Member Author

/cc @mAAdhaTTah @LeaVerou @Golmote

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Mar 3, 2019

It works decently with TypeScript’s checkJs, although that doesn’t yet support @memberof.

@RunDevelopment
Copy link
Member Author

I looked a little into this and there don't seem to be any workarounds for this, are there?

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Mar 3, 2019

The only solution would be if TypeScript would add support for @memberof or if we would migrate portions of this project to TypeScript.

@RunDevelopment
Copy link
Member Author

Well, there is an issue so the hope is there...

I'm actually quite a fan of TS but I think that:

  1. Porting the project for the sake of JS-Check isn't worth it.
  2. TS isn't necessary for Prism.

Also, TS is not as popular as JS, so some people might get held back from contributing.

Copy link
Contributor

@ExE-Boss ExE-Boss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few improvements to the JSDoc:

components/prism-core.doc.js Outdated Show resolved Hide resolved
components/prism-core.js Outdated Show resolved Hide resolved
components/prism-core.js Outdated Show resolved Hide resolved
components/prism-core.js Outdated Show resolved Hide resolved
components/prism-core.js Show resolved Hide resolved
Copy link
Contributor

@ExE-Boss ExE-Boss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer using the simple string[] form instead of Array<string>.

examples.js Outdated Show resolved Hide resolved
tests/helper/test-case.js Outdated Show resolved Hide resolved
tests/helper/test-discovery.js Outdated Show resolved Hide resolved
tests/helper/test-discovery.js Outdated Show resolved Hide resolved
tests/helper/test-discovery.js Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Member Author

That's a good idea. I'll change all similar occurrences.

@RunDevelopment
Copy link
Member Author

Ok. It's done.

After this comment, I gave it another try. This time with the goal of just getting something that works even if it barely works.

The main problem with marrying TS and JSDoc was that TS couldn't do namespaces and wasn't smart enough to figure out some things about the structure of Prism instances. That was a year ago. TS has since become a bit smarter and we had a PR that made it easier for TS to figure out Prism. So TS could now understand Prism. One problem solved.
Now namespaces. Screw namespaces! I just didn't use them. Without using namespaces, the generated doc is somewhat odd because all classes and type definitions are part of the global namespace. This isn't really scalable documentation-wise but TS can understand it, so sure why not. We only have 5 types in global space right now, so it isn't a big deal.

After some tinkering and hacking, I finally had something that worked with both TS and JSDoc.
I also used a different theme that is a lot more configurable and just plain better than the previous one.

It finally works and it even looks kinda nice. Demo.

The hacking

I still have a small CSS file to change/fix the theme but it's better now.

The real hacking comes with JSDoc's parser. It can't deal with some TS syntax (e.g. intersection types and type imports) and will throw an error.
To fix that, I added a handler for JSDoc parser events that will just remove all type imports via a small regex and hopes that it works then. This is nothing more than a hack but it works and I don't plan to change anything about it because the JSDoc team doesn't intend to support this syntax, so this is the best I can do.

TS

One of the main intentions was to get good type checking, so did that turnout?

Amazing. It works really well. You have to kinda work around some issue but it's doable. My goal of moving Prism toward strict typing via doc comments is almost fulfilled. There are only 2 type problems left in Core. For that alone, I want to see this merged.

Other nice things

Documentation is opt-in. You can add doc comments however you like and for whatever you like. Only if you mark a doc comment as @public will it be part of the generated documentation site.

Thanks to the new theme, we now have a search bar and we can add your own links to the nav.

Not so nice things

The doc site includes a "jump to source" function. This means that every change to Core will result in changes in the doc site. To mitigate this a little, the gulp docs task is not part of the default build task. If you make changes to the doc, you have to rebuild the site manually. Not nice, but not too bad either.

Closing thoughts

It's not perfect but it works. You can write some doc and it will generate a nice site for you.

I criticize JSDoc and my solutions to all the problems I had to deal with a lot but I'm actually really happy with this result.

Let me know what you think.
Awaiting review.

package.json Outdated Show resolved Hide resolved
@mAAdhaTTah
Copy link
Member

In another project, I actually wrote everything in JS and managed to generate .d.ts files from them. If we're going to put all this work into writing & maintaining the documentation, is it possible to do that here? Not a requirement for this PR, by any means, just throwing it out there.

@RunDevelopment
Copy link
Member Author

is it possible to do that here?

Should be possible. Let's make an issue of this after this PR is through.

@mAAdhaTTah
Copy link
Member

I'm generally ok with punting off defining TokenStream to a future PR and not blocking this on it.

@RunDevelopment
Copy link
Member Author

Well, if we really narrow down the definition of a Token after this, then that's a breaking change.

Actually, let's just take the current definition of Token and TokenStream as they are in here right now. This will resolve #2071.

@mAAdhaTTah
Copy link
Member

I'm definitely on board with these API docs in general. I would love to eventually get to a world where we generate both types of docs into a single cohesive website, but this works for now.

@RunDevelopment
Copy link
Member Author

I should probably point out that we now have API doc, but I haven't actually linked to it yet... Let just me do that real quick.

@RunDevelopment
Copy link
Member Author

Minor change: I narrowed down the type definition of a Token (reason).
I'm gonna merge now.

@RunDevelopment RunDevelopment merged commit 4ff555b into PrismJS:master Jun 27, 2020
quentinvernot pushed a commit to TankerHQ/prismjs that referenced this pull request Sep 11, 2020
@RunDevelopment RunDevelopment deleted the jsdoc branch October 2, 2020 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSDoc: Generated documentation Docs: Extending Prism page does not mention Prism.languages.extend
4 participants