Add RunKit embeds to Node documentation [WIP]

[tolmasky]

This addresses https://github.com/nodejs/node/issues/21723.

This is functionally complete and only has a few cosmetic changes to be made hopefully, namely, choosing the the node examples to turn on in this initial run, and separately choosing the right syntax highlighting theme. I figured it was worth starting the discussion now as it touched a few minor other things, namely:

1. This change changes nodejs.org to syntax highlight examples at compile time instead of at runtime in the browser.

   Previous to this change, the syntax highlighting on nodejs.org was done at runtime using two scripts that were included in the repo in their minified form, sh_javascript.min.js and sh_main.min.js (which I believe remained untouched since they were originally committed 5 years ago). This made the the RunKit process a bit trickier than necessary, as we would have to first wait for the highlighting to finish before swapping in the RunKit embed. This had a number of other downsides though:

   1. Anyone with JavaScript turned off, or on a slow connection, would see the documentation without syntax highlighting.
   2. Only JavaScript was syntax highlighted.
   3. It is hard to modify syntax highlighting at all due to the minified nature of the scripts.
   4. There was unnecessary load and work done on the user's browser.

   With this change, we're using the standard highlight.js library, which makes it easy to change theme, and additionally means the many many C++ examples on nodejs.org are now syntax highlighted as well (hurray!). Beyond that, the examples will be highlighted instantly of course and it's kind of cool that with the RunKit change most people will actually be loading less JavaScript.

2. As part of the change above, I've taken the standard hopscotch theme.

   I have no particular strong feelings either way about this. Hopscotch seemed to fit well and I was going to translate the existing theme, but it seemed to only highlight keywords as green. I am happy to just do that as well, but hopscotch is very subtle too and, especially for the C++ examples for example, enhances readability. Again, I'm happy to directly translate the existing CSS, or alternatively, if anyone wants to just suggest any of the many many existing highlight.js themes I'm happy to switch to that.

OK, with that out of the way, the meat of the change:

This introduces the ability to add "runkit" to code blocks, turning them into runnable examples. For example:

    ```javascript runkit
    console.log("This example is runnable!")
    ```

The nice thing about this is that the examples continue to syntax highlight correctly in Github as Github just ignores everything after the initial language tag. As explained in the above bug, we're hoping to make this a major part of the full API redesign, but want to do some initial tests (scaling, etc.) on the current site. Currently this commit only includes a crypto example we know to work well, but we're hopefully going to be turning on around 5 - 10 total examples, which we'll be updating this PR with in the next couple of days.

A quick note on the way we've added RunKit: since there is only one template file used for every page, I've created a super tiny script that checks whether any example is runnable on load, and only then adds the runkit script tag. Again, since we're removing much more JavaScript than this already with the syntax highlighting change described above, this should improve load time overall. When we make the full API redesign, the ideal would be to include a defer-ed runkit script tag on pages that use it, and leave it out completely on those that don't.

Checklist

• [x] make -j4 test (UNIX), or vcbuild test (Windows) passes
• [x] tests and/or benchmarks are included
• [x] documentation is changed or added
• [x] commit message follows commit guidelines

[Trott]

@nodejs/documentation @rubys

[vsemozhetbyt]

Does eslint-plugin-markdown still lint code blocks with runkit label?

[vsemozhetbyt]

Does eslint-plugin-markdown still lint code blocks with runkit label?

From my local tests, no. So we need to address this upstream with a proposal for eslint-plugin-markdown or find another way to mark the blocks.

[tolmasky]

I'll take a quick look at it, hopefully there's some setting I can do on our end that doesn't require changing and then upgrading the package.

[tolmasky]

There doesn't appear to be a simple way to do this, but it is a very simple change in the eslint-plugin-markdown code. I've filed the issue here and will be submitting the PR today: https://github.com/eslint/eslint-plugin-markdown/issues/98

[rubys]

The two build failures can be reproduced locally by using the following command:

make lint-js test-doc

Essentially what this means is that tools/doc/html.js needs to conform to node.js coding standards, and an updated tools/doc/package-lock.json needs to be committed.

[tolmasky]

@rubys do you know what is failing now? Everything checks out when I run locally (except for "missing subsystem"?). The title formatting seems to flip back and forth (and I do not get it locally). I did a force push to rebase with master, is there any chance that confuses travis?

[vsemozhetbyt]

It is new commit linting in action:

  ✖  4101442a07256c38bf05149efc862b61970e5a1b
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✖  0:0      Missing subsystem.                        subsystem
     ✖  0:31     Do not use punctuation at end of title.   title-format
     ✔  0:0      Title is <= 50 columns.                   title-length

Just edit the first commit title to something like:

doc,tools: add RunKit embeds to documentation

[tolmasky]

@vsemozhetbyt: thanks!

[vsemozhetbyt]

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1228/

[tolmasky]

Sorry to turn this into node build system 101 for me, but the current failures seem unrelated to anything in my branch (and locally master fails make -j2 test as well). However, I see other PRs passing, so I'm not sure what I'm doing wrong.

[vsemozhetbyt]

If you mean the status under this PR, this is often flaky. If you check CI itself, you can see it is OK:

https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1228/

https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1228/pipeline

[Trott]

The failure of test-buffer-alloc on Travis was unrelated to this change and was fixed in another commit on master already. (That one was my fault. Sorry!) The other stuff here is green. So I think this is good, at least as far as tests go.

[Trott]

(To fix your local failures, you probably just need to rebase against current upstream/master. If that doesn't do it, the next most obvious likely problem is a bug in macOS Mojave that causes two or three tests to fail. And if that's not it...let's talk more.)

[tolmasky]

Alright, so I have just issued a pull request to eslint-plugin-markdown to add the fix I implemented.

Currently this branch uses a fork of eslint-plugin-markdown I published on npm to test to make sure these changes would work, and it appears they do. I'm going to give it a few days to see if the PR to eslint-plugin-markdown is accepted, but given that there has been relatively little activity on that repo for a while (and the original bug was never responded to) I'm curious if it would be acceptable to use our fork here if the PR is not addressed. If not, I can try to find another workaround.

[tolmasky]

Quick update: eslint-plugin-markdown has merged the Pull Request, and they believe they'll roll a release over the weekend, at which point I'll be able to include it here.

[btmills]

I just published [email protected], which includes https://github.com/eslint/eslint-plugin-markdown/pull/99 :ship:

[tolmasky]

I've now changed it to use [email protected]. Along with this, I've updated update-eslint.sh to refer to ^1.0.0-rc.0 since next no longer refers to anything, and I don't think we want to rely on latest (but let me know if you prefer me to change it to latest). As part of this, I had to upgrade to eslint 5.8.0 because its non-trivial to update just eslint-plugin-markdown with the script, so that's why the PR has gotten much larger. I've tried to contain the update to its own commit, and if I make more changes I'll rebase it in a way where they hopefully come in before that, so that its possible to easily see all the "logical" changes and then have the big vendored libraries change come last.

As all tests seem to now pass we'll try to get the cosmetic changes in this week (choosing which embeds, the theme, etc).

[vsemozhetbyt]

It seems ESLint will be updated soon on master: https://github.com/nodejs/node/pull/23904

[tolmasky]

Ah that's great @vsemozhetbyt, I can easily rebase on master when that happens. I'll try to do it as soon as it happens, otherwise running update-eslint.sh will still generate a different result unfortunately due to dependencies changing.

[vsemozhetbyt]

#23904 landed)

[tolmasky]

Thanks for the heads up @vsemozhetbyt! I've rebased on top of master, which has certainly reduced the file change count (although there's still a lot due to going from [email protected] to [email protected]). I might fix up the history to get rid of the intermediate commit that had us using our own fork later today. Going to wait to make sure tests still pass first.

[vsemozhetbyt]

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1410/

[tolmasky]

I believe this PR is now ready for review. We've gone ahead and chosen the querystring page in its entirety to convert to embeds since it 1) contains very few examples so still fulfills are minimal footprint requirements for this initial test run and 2) allows us to have all the examples on one page to best test what such an experience might be like. Here is a quick before and after example:

We chose to just match RunKit's styles (vs. using hopscotch), which seem to fit pretty well with the current nodejs api colors. Again, with this change even the C++ will now be highlighted:

Additionally, as mentioned above, the syntax highlighting now takes places during the creation of the API docs instead of being a script that is run on the client, so mobile and slow network users will see highlighted code immediately, and the site experience is now more or less identical with javascript turned off.

I have uploaded a fully working build of the API docs here for anyone to try out: https://runkitdev.github.io/nodejs-api-docs/querystring.html

Please let us know if any changes are desired.

Thanks!

[vsemozhetbyt]

Are the big error blocks Error: Cannot find module 'urlcharset' intended?

[vsemozhetbyt]

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1652/

[tolmasky]

@vsemozhetbyt Sorry, hit send just a little too soon, still spinning this up. Should be ready momentarily!

[tolmasky]

OK, should be up now, but this actually reminds me that I believe in an offline discussion we had chatted about trying to do this all without modules, so will probably be committing once more to this branch with a change for that.

[devsnek]

can we disable the run by default stuff? it makes the page jump around bad enough on this small page, and it will be even worse on pages like buffer.

[tolmasky]

Yeah its very easy to disable auto-run, might be possible to make it not jump around when its on too.