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

doc: add links to alternative versions of doc #10958

Closed
wants to merge 10 commits into from

Conversation

chris--young
Copy link

@chris--young chris--young commented Jan 23, 2017

Each page of the api documentation should have links to other versions
of the same page. This will make it easier to switch between the current
"live" release at nodejs.org and LTS versions.

Fixes: #10726

screen shot 2017-05-09 at 3 24 36 pm

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc, tools

@mscdex mscdex added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jan 23, 2017
@evanlucas
Copy link
Contributor

This is great @chris--young. Thanks for doing this.

const a = (v) => `<a href="${href(v)}">v${v}</a>`;
const as = (vs) => vs.filter(lte).map(a).join(' / ');

const lts = as(['0.12.x', '4.x', '6.x']);
Copy link
Contributor

Choose a reason for hiding this comment

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

0.12 is no longer in maintenance mode, so I would say remove it from here.

Copy link
Author

Choose a reason for hiding this comment

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

I've moved v0.12 into the "Unsupported" section

if (lts.length)
html += '<b>LTS:</b> ' + lts;

const unsupported = as(['0.10.x', '5.x', '7.x']);
Copy link
Contributor

Choose a reason for hiding this comment

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

v7.x is still a supported version, so calling it Unsupported would be confusing IMO

Copy link
Author

Choose a reason for hiding this comment

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

v7.x is now labeled as Current

@Trott
Copy link
Member

Trott commented Jan 31, 2017

@evanlucas It looks like @chris--young has tried to address your comments. Can you PTAL when you get a chance and (if appropriate) update your review status?

@Trott
Copy link
Member

Trott commented Jan 31, 2017

This could use some more reviewers. Looping in the people who gave it a +1 commented in the original issue: @MylesBorins @benjamingr @sam-github

Also: @nodejs/documentation

@sam-github
Copy link
Contributor

I didn't give it a +1 in the original issue, at least I see no signs I did :-). I just CCed it to the website group.

The docs we have right now for 7.x are better to use for 4.x than than the 4.x docs were.

This is likely to misdirect people into thinking that node is like express, http://expressjs.com/en/4x/api.html, where you should always look at the docs for the version you are using.

I won't object if others find this useful, but I also won't approve.

@chris--young
Copy link
Author

@sam-github any suggestions for how I can salvage this? I was thinking if I restyled the links it might be clearer that you're probably looking for the latest version (7.x) and that 0.10x - 6.x are older versions.

@sam-github
Copy link
Contributor

@chris--young I'm +/- 0. You could try to convince me, then you'd have two approvals once you address @evanlucas 's comments :-). The PR says what it does, but doesn't say why ("I was thinking it'd be nice" doesn't count). What's the use-case?

@nodejs/documentation @nodejs/website PTAL

@Trott
Copy link
Member

Trott commented Feb 10, 2017

What's the use-case?

I would think one use case is if some API was the subject of a breaking change in 7.x but you're using 4.x or 6.x, you'll want to switch to the earlier docs easily.

@sam-github
Copy link
Contributor

Except you won't really know that was a breaking change, unless it was the intoduction or removal of an entire API (because we don't document major changes other than that). And many of the doc improvements apply to older releases. We'd have to start being really rigorous about backporting docs. Currently, the don't backport clean to 4.x because of the .markdown to .md rename, so 4.x docs are no longer the best reference for the features in 4.x. I guess I'm saying I don't think we maintain the LTS docs rigorously enough, and we don't break the node API very much, so this isn't as useful as http://expressjs.com/en/4x/api.html vs http://expressjs.com/en/3x/api.html, and it may be dangerous to suggest we do.

On the other hand, maybe this will force us to backport all doc improvements more consistently, which would be a good thing.

@toddself
Copy link
Contributor

I filed the original issue for this. @sam-github the idea isn't to be able to figure out what is different but to easily be able to find relevant documentation. Each major version is a major version specifically because something in an outward API changed. If i am writing code for 4.x (or even 6.x) I need to be able to quickly look up docs for that version. Right now this is hard - if you google something you always get latest. It should be trivial to get linked to the version of the docs for the API you are working with

The node 7 docs are not helpful when writing code for node 4 (or even node .12 if you are still supporting such a thing).

@@ -1,5 +1,7 @@
# Addons

<!--doc_created=v0.10.0-->
Copy link
Contributor

Choose a reason for hiding this comment

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

doc sounds superfluous to me. How about "introduced_in"?

Copy link
Author

Choose a reason for hiding this comment

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

@thefourtheye: seems reasonable, just made that change

@sam-github
Copy link
Contributor

Each major version is a major version specifically because something in an outward API changed.

@toddself spelling corrections in the thrown Error messages are semver-major

OK, I don't object to this, it sounds easy and like some people will find it useful, but I should point out that when the documentation is improved on master, as it often is, for features going all the way back to before LTS (including docs of APIs that existed in 4.x but were not doced), that does NOT guarantee that the docs get backported.

@chris--young chris--young force-pushed the doc-version-links branch 2 times, most recently from 991e273 to ea244fa Compare March 8, 2017 02:28
@chris--young
Copy link
Author

@evanlucas please re-review

@@ -1,5 +1,7 @@
# Crypto

<!--introduced_in=v0.10.0-->
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this is correct. (https://nodejs.org/docs/v0.3.6/api/crypto.html) crypto has been around for a while. https://nodejs.org/en/download/releases/ is a useful page to check :]

Copy link
Author

Choose a reason for hiding this comment

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

Oh cool, I had only been looking at https://nodejs.org/dist/ and must have messed up a few versions. I'll go back over all the docs and make sure these tags are the right version number.

@@ -1,5 +1,7 @@
# Debugger

<!--introduced_in=v0.10.0-->
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is correct either? https://nodejs.org/dist/v0.9.12/docs/api/debugger.html

const a = (v) => `<a href="${href(v)}">v${v}</a>`;
const as = (vs) => vs.filter(lte).map(a).join(' / ');

html += 'View another version of this page <b>Current:</b> ' + a('7.x');
Copy link
Contributor

Choose a reason for hiding this comment

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

so one concern I have here is what will this look like when Node 8 is released? It will be released as a "Current" release line for a while before going LTS. With that, there will be a few months of layover where Node 7 is still also a "Current" release.

@tinder-ydong
Copy link

had same problem finding different versions of docs, and feel it'd be really convenient to add links to different versions

@chris--young
Copy link
Author

@BridgeAR I don't really like the idea of merging it when we know it's broken on mobile. If you can give me till the end of the weekend i'll get it to where it needs to be.

Node.js Addons are dynamically-linked shared objects, written in C++, that
<!--introduced_in=v0.10.0-->

Node.js Addons are dynamically-linked shared objects, written in C or C++, that
Copy link
Contributor

Choose a reason for hiding this comment

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

This commit explicitly changed this to C++ abfd4bf

@@ -31,6 +31,7 @@ const typeParser = require('./type-parser.js');
module.exports = toHTML;

const STABILITY_TEXT_REG_EXP = /(.*:)\s*(\d)([\s\S]*)/;
const DOC_CREATED_REG_EXP = /<!--introduced_in=v([0-9]+).([0-9]+).([0-9]+)-->/;
Copy link
Contributor

Choose a reason for hiding this comment

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

The dots need to be escaped. Also, you can optionally relax the RegEx to accept optional spaces.

return html;
}

function lte(v) {
Copy link
Contributor

Choose a reason for hiding this comment

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

String based comparison with v.num is not good enough. Why not split it at dots and the do integer comparison?

let html = '';

if (!docCreated) {
console.error('Failed to add alternative version links');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: If the filename is also included, it would help in debugging.

@@ -31,6 +31,7 @@ const typeParser = require('./type-parser.js');
module.exports = toHTML;

const STABILITY_TEXT_REG_EXP = /(.*:)\s*(\d)([\s\S]*)/;
const DOC_CREATED_REG_EXP = /<!--introduced_in( )?=( )?v([0-9]+)\.([0-9]+)\.([0-9]+)-->/;
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking more like

const DOC_CREATED_REG_EXP = /<!--\s*introduced_in\s*=\s*v([0-9]+)\.([0-9]+)\.([0-9]+)\s*-->/;

{ num: '5.x' },
{ num: '4.x', lts: true },
{ num: '0.12.x' },
{ num: '0.10.x' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Extra comma at the end.

@chris--young
Copy link
Author

@BridgeAR should be good to go now, unless anyone has objections

tniessen pushed a commit to tniessen/node that referenced this pull request Aug 28, 2017
Each page of the API documentation should have links to other versions
of the same page. This will make it easier to switch between the current
"live" release at nodejs.org and LTS versions.

PR-URL: nodejs#10958
Fixes: nodejs#10726
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@tniessen
Copy link
Member

Landed in cacce30, thank you for the contribution! 🎉

@tniessen tniessen closed this Aug 28, 2017
@targos
Copy link
Member

targos commented Aug 28, 2017

A little linting error was introduced with this commit. Fix: #15063

ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
Each page of the API documentation should have links to other versions
of the same page. This will make it easier to switch between the current
"live" release at nodejs.org and LTS versions.

PR-URL: nodejs/node#10958
Fixes: nodejs/node#10726
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
Each page of the API documentation should have links to other versions
of the same page. This will make it easier to switch between the current
"live" release at nodejs.org and LTS versions.

PR-URL: nodejs/node#10958
Fixes: nodejs/node#10726
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Aug 31, 2017
Each page of the API documentation should have links to other versions
of the same page. This will make it easier to switch between the current
"live" release at nodejs.org and LTS versions.

PR-URL: nodejs#10958
Fixes: nodejs#10726
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
Each page of the API documentation should have links to other versions
of the same page. This will make it easier to switch between the current
"live" release at nodejs.org and LTS versions.

PR-URL: #10958
Fixes: #10726
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
Each page of the API documentation should have links to other versions
of the same page. This will make it easier to switch between the current
"live" release at nodejs.org and LTS versions.

PR-URL: #10958
Fixes: #10726
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@refack
Copy link
Contributor

refack commented Sep 20, 2017

@chris--young IMHO it would be nice to have this in 6 so as to enable cross referencing.
(Should be backported with the relevant fixes #15063, and #15420)

@chris--young
Copy link
Author

@MylesBorins yea seems like a good idea. will open a pr soon

chris--young pushed a commit to chris--young/node that referenced this pull request Sep 29, 2017
Each page of the API documentation should have links to other versions
of the same page. This will make it easier to switch between the current
"live" release at nodejs.org and LTS versions.

PR-URL: nodejs#10958
Fixes: nodejs#10726
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
Each page of the API documentation should have links to other versions
of the same page. This will make it easier to switch between the current
"live" release at nodejs.org and LTS versions.

Backport-PR-URL: #15670
PR-URL: #10958
Fixes: #10726
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node js api document should link to LTS release from each page