doc: add runkit embeds

[devsnek]

This is a rework of #22831

Fixes #21723

[bnoordhuis]

The commit log could perhaps provide a little more detail but otherwise LGTM.

[BridgeAR]

🎊

[benjamingr]

This is pretty cool, I hope the legal aspect has been taken care of in terms of the foundation and runkit :]

[devsnek]

I wasn't quite aware that there was a legal aspect 👀. As far as I can tell, this just uses runkit's public api: https://runkit.com/docs/embed

[richardlau]

I prefer the way #22831 marked runkit after the "```js" fence rather than inside it like it is here. This comment is visible when rendered as markdown (and maybe in the json version of the docs too?).

[devsnek]

it wouldn't highlight correctly in any editors i tried with that approach.

[BridgeAR]

Should we maybe land this as is for now and try to improve the solution later on?
It seems like it would even be an improvement having the comment in markdown compared to now.

[sam-github]

I don't understand why this comment line is being added into only one of the js examples, is it the intention that it later be added to every single one? That seems overly intrusive.

[benjamingr]

I wasn't quite aware that there was a legal aspect 👀. As far as I can tell, this just uses runkit's public api: https://runkit.com/docs/embed

@nodejs/community-committee are you the appropriate folks to ping here? This is integrating a third party's service into our docs. I just . want to make sure there are no legal complications involved.

[devsnek]

cc @nodejs/tsc re: benjamin's concerns

[Trott]

cc @nodejs/tsc re: benjamin's concerns

I'm unaware of any issue the Foundation would have with us using RunKit in our docs so I'm not sure who to ask. I guess I'd ping @brianwarner ?

[tolmasky]

Found a reference to this in #22831. I think that one is ready to go, happy to combine in whatever way people want too. One of the things I'd definitely like to not see lost from 22831 is that that one first put in a bunch of work to just have the syntax highlighting take place at website creation time, vs. having it be something that runs on user's computers every time they load the page (this is unrelated to the embed stuff completely, but was a nice side-effect of the choices made there to enable using "```javascript runkit").

Edit: Just now seeing that this is based on that one, so it might have these things already. In that case no qualms on my end. Apologies, thought this was a separate parallel version.

[devsnek]

@tolmasky i used your pr as the base, so its fairly similar and it does perform highlighting at build time.

I would really like to mark the runkit embeds with comments above the codeblocks, but that information seems to be lost in the markdown parser that we use. I'm definitely open to suggestions on better ways than the current // RUNKIT-ENABLE

[tolmasky]

I am fine either way, that being said, I ran into the same issue with editors and I just added support for the multi-word code-fence to my editor's markdown mode. This is because multi-word code-fences are actually correct according the markdown standard. Unfortunately I use a niche text editor so that doesn't help much people. However, it might be easier to just fix the markdown modes for Sublime and Atom or whatever than changing our detection method, especially since its usually a one-line fix for most editor's modes, literally changing something like /```([^\s]*)\n/ to /```([^\s]*)[^\n]*\n/.

[Trott]

Totally OK with going the fix-the-editors route but I'm also fine landing this as-is and updating again after a fix-the-editors campaign.

[tolmasky]

I am totally cool with that too, just if you're curious (but definitely not pushing for or wanting to delay further), I remembered that I did a similar comment strategy with immutable.js's docs with the only difference being the comment was outside the code example (<!-- runkit:activate -->) that way the code example itself wouldn't have to be mutated. Again, providing simply for reference.

Edit: more helpful link: https://github.com/immutable-js/immutable-js/pull/1261/files#diff-04c6e90faac2675aa89e2176d2eec7d8R44

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28108/

[BridgeAR]

I marked this as author ready, since the comment does not seem blocking.

[devsnek]

I thought we were waiting to hear from commcomm or something about third party APIs in our docs. If not I can merge this.

[BridgeAR]

@nodejs/community-committee @nodejs/tsc could you please have a look at the legal question raised here #30241 (comment)?

[mcollina]

I think this is worthwhile to be addressed in a tsc meeting.

[tolmasky]

I certainly don't want to dissuade anyone from discussing this again, but I do want to point out that these changes originated from numerous discussions in commcomm meetings (as you can see from the authors of the original bug).

[amiller-gh]

Re:

This is pretty cool, I hope the legal aspect has been taken care of in terms of the foundation and runkit :]

and

I'm unaware of any issue the Foundation would have with us using RunKit in our docs so I'm not sure who to ask. I guess I'd ping @brianwarner ?

We talked about this extensively on the CommComm side of things and found no issue with this landing, but work stalled independently of this PR. There should be no legal concerns since RunKit makes no claims of ownership to any content uploaded to its service, and has a very generous blanket Property Rights clause in its terms (see: https://runkit.com/s/terms). As @devsnek mentioned, this uses RunKit's public api and has no more or fewer legal concerns than using Gatsby (see: nodejs-dev), self-rolled Wordpress (like the Foundation), etc.

I'm +1 on this landing – and @devsnek, would love to see a version of this in @nodejs/nodejs-dev soon! Lets talk 🙂

[mcollina]

Is there a summary of this discussion? I’m not opposed, but I’m usually skeptical of adding an external service.

[mcollina]

What does the rest of @nodejs/tsc think?

[sam-github]

Note: adding runkit is proposed to address nodejs/website-redesign#12 -- which I can't find linked here.

[mcollina]

How is the Node.js version specified? The docs points to a specific version of Node.js. How is Runkit picking the right version of Node.js to execute?

[mhdawson]

There are some concerns from the TSC.

• Its the first external dependency and there was some discussion about the risk of code/JavaScript
  that comes from outside the repo
  • executing JavaScript code from their domain in our region.
  • tracking our users
  • what happens if there are outages (seems like it degrades nicely to just show the code)
  • redirecting users
  • do we need to warn people that some content is coming from outside
  • can we make it optional to enable runkit.

Is it possible for somebody who is advocating from runkit come to a TSC meeting to discuss (might also be good if they could line up somebody from runkit as well to be there).

[jasnell]

I share the concerns about this that have been voiced around loading and running third party code. I would be much happier if we could find a good way of making it entirely opt in only.

[tolmasky]

I wasn't a part of these discussions in the nodejs.dev meetings but I found mentions in the notes:
https://github.com/nodejs/nodejs.dev/search?q=runkit&unscoped_q=runkit

Hi @joesepi, allow me to provide some context that will hopefully alleviate your concerns about the intrusiveness of this:

1. The original intent of this commit was to only enable one code snippet in order to have one live and be able to test a few things on our server that we otherwise can't.
2. After that, there was going to be a separate PR that would enable more code snippets.
3. However, it was always the case that not every code snippet would necessarily make sense to be runnable. For example, there are many code snippets in the documentation that are "invalid" (for example, some rely on the assumption of previous implied code, etc.), so there needs to be a way to easily configure code snippets as runnable or not.
4. The original PR had the "runnable" configuration in the code fence ( ```javascript runkit) instead of the code itself. This is valid markdown and renders fine in GitHub for example (you can see this in action here ), however the author of this revised commit noted that it didn't properly syntax highlight in some markdown editors. This can be remedied in a fairly straight-forward way by updating those plugins, but until that the decision on the part of the author of the revised PR was to keep use this comment method instead -- which I think is perfectly acceptable.

[tolmasky]

@mcollina RunKit is fully capable of supporting all the versions of node on the node site. It's been a while since I ran this PR, but I can make sure its checking the version on the page and assigning the right one and fix it if it isn't.

[mcollina]

RunKit is fully capable of supporting all the versions of node on the node site. It's been a while since I ran this PR, but I can make sure its checking the version on the page and assigning the right one and fix it if it isn't.

I don't see any code related to that in this PR. How is this info passed down to RunKit?

[tolmasky]

@mhdawson / @jasnell

I am from RunKit and can come to the TSC meeting if you want. For some background, we demonstrated this in Germany last year at the Node collaborators summit in Berlin back in 2018, and then worked closely with the website redesign committee as well as comm comm. We believed they were the only stakeholders but are certainly happy to talk to anyone TSC or anyone else and answer any questions. In order to make this more efficient though, are there any other groups that it would be helpful to pro-actively reach out to (I am just not super familiar with thee organization structure of node and don't know if there would be a third interested party).

To answer some of the questions here and just for reference:

1. The outage situation is indeed specifically handled in this PR. In fact, this PR goes above and beyond in that it compiles the highlight.js syntax coloring as a build step (as opposed to the current system that does so on the client), so if you now run the nodejs site with javascript turned off, you will not only see no degradation from a lack of RunKit, it will actually render nicer than the current situation where the code will also not be syntax highlighted. With JS turned on, all the pages loaded faster as every page load didn't generate several dozen highlight js dom mutations.
2. The RunKit embed I think makes it clear that the content is coming from the outside, but I may have misunderstood this question.
3. It's certainly possible to make RunKit optional, but I'm not sure what the ergonomics of that would be. We wouldn't want some "site-wide settings" page or something, and one of the original goals of this feature was to make Node more accessible to first time users. The goal is to make it so upon learning about node you can immediately start playing around with it, instead of having to install node, open the terminal, copy-paste the code in, etc.

I think this "beginner" use case becomes more relevant to even experts when you consider unstable releases and nightlies. Again, this was meant to be an initial test run (which will support all the released versions of node), but to give you an idea of what we wanted to eventually enable, we are currently preparing RunKit to support not just all the released versions but also nightlies. We think it would be really helpful to be able to one day point people at the nightly docs to try out cool new features (whether they be node platform features or v8 features or new syntax, etc.). Currently, it requires a lot of work to experiment with just-released features only available on nightlies (you have to download manually as its not support in "version managers" like nvm, etc.), which unfortunately limits the pre-release exposure and thus bug reporting. We think it would be really cool to just be able to say something like "hey we just enabled BigInt in the node nightlies, you can see how it works with our existing APIs here -- link".

[tolmasky]

RunKit is fully capable of supporting all the versions of node on the node site. It's been a while since I ran this PR, but I can make sure its checking the version on the page and assigning the right one and fix it if it isn't.

I don't see any code related to that in this PR. How is this info passed down to RunKit?

It might have gotten lost from the old PR (or perhaps it was always an uncaught bug), but it should be in the instantiation code, you can just pass nodeVersion to the constructor. I can't get to it right now but I'll check by end of day and submit the change.

[tolmasky]

@mcollina: I've gone ahead and made the change to make it choose the right version of node: runkit-forks@effd602. It hasn't made it's way into this PR yet since it needs to be applied to this branch first.

[mcollina]

@tolmasky it seems you are using .x.x as the version numbers. However those would need to be specified as our docs targets individual versions (the previous docs are not unpublished).

[tolmasky]

@mcollina Correct, as of current RunKit operation you will get the closest major version match. We won't support minor/patch level for a little while longer (probably the same time we ship nightly support). I believe this was discussed in the previous meetings and deemed acceptable, at least for this initial one-embed test.

[mcollina]

@mcollina Correct, as of current RunKit operation you will get the closest major version match. We won't support minor/patch level for a little while longer (probably the same time we ship nightly support). I

This will (possibly) create a situation where a snippet on v15.0.0 will produce a different output depending on the latest version of the line.

I believe this was discussed in the previous meetings and deemed acceptable, at least for this initial one-embed test.

I asked for some details about the decisions regarding this PR, and I still did not get an answer (see #30241 (comment)).

[tolmasky]

@mcollina Correct, as of current RunKit operation you will get the closest major version match. We won't support minor/patch level for a little while longer (probably the same time we ship nightly support). I

This will (possibly) create a situation where a snippet on v15.0.0 will produce a different output depending on the latest version of the line.

Yeah the plan is definitely to have support for this, and I think the current way docs are set up is such that it wouldn't go live until 14.0.0 so at least for that initial version it would be fine and we in theory might even have support for the more sophisticated behavior before the 14.0.0 launch and even more likely before 14.0.1 when it would be the first instance of potentially experiencing this.

I believe this was discussed in the previous meetings and deemed acceptable, at least for this initial one-embed test.

I asked for some details about the decisions regarding this PR, and I still did not get an answer (see #30241 (comment)).

I'm not a member of any of the node committees so I unfortunately don't know where the commcomm meeting notes or node-contirbutor-summit videos are stored. I could start poking around but if there's someone who knows where these are it would probably be faster.

[mcollina]

Yeah the plan is definitely to have support for this, and I think the current way docs are set up is such that it wouldn't go live until 14.0.0 so at least for that initial version it would be fine and we in theory might even have support for the more sophisticated behavior before the 14.0.0 launch and even more likely before 14.0.1 when it would be the first instance of potentially experiencing this.

The way this is currently tagged implies it can land in 13 and potentially be backported down to 10.

[tolmasky]

Yeah the plan is definitely to have support for this, and I think the current way docs are set up is such that it wouldn't go live until 14.0.0 so at least for that initial version it would be fine and we in theory might even have support for the more sophisticated behavior before the 14.0.0 launch and even more likely before 14.0.1 when it would be the first instance of potentially experiencing this.

The way this is currently tagged implies it can land in 13 and potentially be backported down to 10.

In that case I suppose there is a chance that if future minor or patch changes to the node 10/12/13 affect the behavior of the hash function for the crypto library then the scenario you describe could happen. If we want to block the PR until we have comprehensive minor and patch support across all node versions in RunKit that is certainly fine. We could alternatively tag this to just be for 14, or we could do an in-js test to only show the RunKit embed if the version on RunKit exactly matches the version on the docs page. So something like if ((RunKit.supportedVersions[version.split(".")[0]].indexOf(version) !== -1) or something. This would of course require us to write that feature on our end but your call however you'd like to proceed with any of this.

[devsnek]

@tolmasky i will pull your changes in shortly.

[jasnell]

@tolmasky it looks like the technical concerns are well under control but I (and I know a few others on the TSC) would definitely like to see the concerns around possible user tracking addressed. Specifically, what information about visiting users does RunKit collect and how is that information used? Specifically, can you point us to any relevant privacy and GDPR-related policies?

[mhdawson]

@tolmasky I think the meeting this week is "full" but lets get you invited to the meeting on the Jan 22nd at 3PM EST so that we have have a live discussion of the concerns/issues which remain at that point.

[mhdawson]

@tolmasky once you confirm you can make the meeting next week we'll sync on getting you the links/passwords.

[tolmasky]

@mhdawson I'm confirming that I intend to attend. That being said, we are currently full term so there is a chance that my wife will be be in labor sometime near then and I'll be unavailable (and perhaps without being able to give proper notice). The date should be fine for our schedule but just want to give a heads up about that possibility now.

@jasnell We intend to be completely cookie-less by the release of this feature. I'd like to give comprehensive details at the TSC meeting and answer any questions as well, but for a short historical summary here: RunKit used to to create a "pre user" if you weren't logged in and used an embed, that way if you did log in later, or sign up later, the various edits you had made would be available to you. This ended up being more trouble than it was worth, and, while for some people a pleasant surprise upon discovery, most people were just confused by the feature, so we ended up turning it off. However, this pre-user cookie never got disabled so we currently store a useless cookie that we intend to get rid of (should be live by the meeting on the 22nd). We previously had a demo server up with the RunKit embeds we were going to ship alongside the node page (which mainly dealt with performance issues), but we're about to merge that code this week so it should be all usable by the TSC meeting and I don't think its worth us spinning up the demo server again.

[devsnek]

I'm gonna close this so @tolmasky can take over. Feel free to use my changes.

[tolmasky]

Hi @mhdawson , just checking in because I haven't received an email regarding this yet.

[mhdawson]

@tolmasky, Sent you email with password as well as link to TSC meeting issue that has zoom link