Skip to content

Conversation

strugee
Copy link
Contributor

@strugee strugee commented Jul 11, 2017

Build manpages alongside JSON and HTML documentation. WIP to solicit feedback - does this direction make sense? Does anyone have opinions already on how this looks?

Fixes #8903

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

doc

Open issues
  • Tests
  • Handle YAML data being where stability information usually is (would pull domain, punycode off the dontBuild list)
  • Handle stability information not being present (would pull many files off the dontBuild list)
  • Should I split up the node_modules change into a separate commit? The diff is kinda unreadable otherwise
  • Move the "don't build this" metadata into the Markdown files themselves
  • node-debugger(3), node-buffer(3), node-n-api(3), node-console(3), node-modules(3), node-process(3), node-timers(3) don't make much sense since they're not require()able modules
  • Reduce the dontBuild list to what it actually should be (for example node-errors(3) really should be built, but crashes right now)
  • Fix node-events(3) SYNOPSIS (and maybe other similar pages?)
  • Some pages (like the OS module docs) duplicate information on how to require() the module
  • Have node(1) point to things like node-debugger(3) when it makes sense
  • Fix SEE ALSO in node(1)
  • Should we have node-roadmap(1) and/or node-all(3), à la zsh?
  • Do some of these belong in separate sections? E.g. node-debugger(1) instead of node-debugger(3)?
  • Does BSDMakefile also need to be updated?
  • The generated files aren't named properly (e.g. http.3 instead of node-http.3)
  • Inline comments

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jul 11, 2017
Makefile Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from the HTML build logic, but it's now duplicated three times. Should we refactor this? I think yes, but my Make-fu is not very good...

Makefile Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we actually need npm install since node_modules is vendored?

Copy link
Member

Choose a reason for hiding this comment

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

would definitely prefer not having npm install here.

tools/doc/man.js Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this correct, given that I wrote this file from scratch?

@mscdex
Copy link
Contributor

mscdex commented Jul 11, 2017

buffer, timers, process, and console are definitely all require()-able.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be before the console.log()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mscdex lol, you'd think :D

I was looking at case 'json': when I wrote that, trying to mirror surrounding code style; perhaps that one's wrong too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll push a fix tomorrow then

tools/doc/man.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are domain, process, punycode, and v8 included here? They are all require()-able.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mscdex a lot of these are here because they had some structural issue that the build tool couldn't handle - stability information in an unexpected place, etc. I'm planning to go back and fix a lot of these (see the "open issues" list in the description)

tools/doc/man.js Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that with this change we now have two Markdown engines in use. This is unideal obviously, but truthfully I don't think it's a huge issue. I also think it probably makes sense to convert other uses of marked to Remark anyway, since the latter has much better tooling. But we can do that later.

tools/doc/man.js Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should make this line not so long but it's 1 AM and I want to go to bed. Tomorrow!

tools/doc/man.js Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually like this style better (and functionally it doesn't super matter since we just throw either way) but it seems like this should probably just pass an Error to next()... if people disagree with that assessment though I'd prefer to keep it this way. It's more concise and obvious what's happening.

tools/doc/man.js Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way of building nodes is kinda fugly but I'm not sure there's a better way. https://github.com/eush77/unist-builder perhaps?

tools/doc/man.js Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really hacky and leaves around empty files. Not good.

@strugee
Copy link
Contributor Author

strugee commented Jul 11, 2017

@mscdex re: buffer, process, etc. being require()able - yes, but you wouldn't actually do that in normal usage, as the SYNOPSIS suggestion currently suggests. I.e. for Buffer,

const buffer = require('buffer');

isn't nearly as helpful as

Buffer.from('string');
Buffer.alloc();
// etc.

@mscdex
Copy link
Contributor

mscdex commented Jul 11, 2017

@strugee I disagree about buffer. There are useful properties and methods that are only available via require('buffer'). Similarly, timers also exports functions that can be useful when dealing with different kinds of timers (although they are currently undocumented).

@refack
Copy link
Contributor

refack commented Jul 11, 2017

/cc @nodejs/documentation

@refack
Copy link
Contributor

refack commented Jul 11, 2017

Since this is a "WIP to solicit feedback" let's talk about webpackeing the new tooling? I see 379 new files, while only 3 are outside of node_modeules. Since you already added the formula to the Makefile it should be straightforward to also pack the new modules and IMHO packing will have several benefits:

  1. easy to ignore by the linter and other static-analysis tools (such and IDEs)
  2. atomic changes
  3. a tiny bit faster git experience
  4. less noisy for grep
  5. easy to cleanup

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 11, 2017

I'm quite worried about how difficult this may be to keep up-to-date?

Perhaps we should start generating docs like libuv does, it generated both webpages and man pages, although the quality of both is a bit less... idk probably not a great solution unless we generate roff syntax from markdown.

@jasnell
Copy link
Member

jasnell commented Jul 12, 2017

This is definitely an interesting idea that is worth exploring, but I do share @Fishrock123's concerns about maintainability. As is, updating the docs and keeping them up to date is a bit of a hassle.

@strugee
Copy link
Contributor Author

strugee commented Jul 12, 2017

@mscdex okay! I don't feel strongly about it. You'd certainly know better than I ;)

Since this is a "WIP to solicit feedback" let's talk about webpackeing the new tooling?

@refack can you clarify what you mean by this? Why would we use Webpack? Even the atomicity problem isn't all that big (at least, AFAICT) since we vendor node_modules in git

@Fishrock123 are you thinking about this code becoming out-of-sync with the documentation's structure? That's definitely a legit concern which you guys will be able to answer better than I :)

I did, however, try to make sure that if something goes wrong, there's at least a good blowup. That's why all the assertions are in there.

idk probably not a great solution unless we generate roff syntax from markdown.

That's what this patch does, unless I'm misunderstanding you?

@refack
Copy link
Contributor

refack commented Jul 12, 2017

@refack can you clarify what you mean by this? Why would we use Webpack? Even the atomicity problem isn't all that big (at least, AFAICT) since we vendor node_modules in git

Just throwing ideas... Having a single file for something that we want to treat as a black box has some benefits:

  • Ignored by tools and devs doing grep
  • Reduce fs pressure on git and IDEs
  • Could use Git LFS for quicker clone/checkouts

@strugee
Copy link
Contributor Author

strugee commented Jul 20, 2017

@Fishrock123 @jasnell so should I keep going with this? Don't want to spend any more time on it if it won't be merged due to maintainability concerns :)

I'm also pretty confused by:

Perhaps we should start generating docs like libuv does, it generated both webpages and man pages, although the quality of both is a bit less... idk probably not a great solution unless we generate roff syntax from markdown.

Unless I'm misunderstanding something this is what this PR already does? The existing Markdown documents are transformed into roff syntax automatically.

@strugee
Copy link
Contributor Author

strugee commented Aug 26, 2017

@Fishrock123 @jasnell ping?

@jasnell
Copy link
Member

jasnell commented Aug 29, 2017

@strugee ... my apologies, I lost track of this. Personally, I love this idea, I'm just concerned about the maintenance. The PR itself is also a bit difficult to review with the added tools dependencies.

So I would say, let's definitely proceed, but can I ask you to split the commits into multiple? One that contains the various new dependencies, and one that contains the new code added? Then I'll take another look.

I really appreciate you working on this!

@jasnell jasnell self-assigned this Aug 29, 2017
@BridgeAR
Copy link
Member

Ping @strugee

@strugee
Copy link
Contributor Author

strugee commented Sep 22, 2017

Hey, sorry! Thanks for the ping. I split up the commit and am hoping to do some polishing on this during the weekend 👍

@strugee
Copy link
Contributor Author

strugee commented Sep 22, 2017

/cc @jasnell

@jasnell
Copy link
Member

jasnell commented Sep 25, 2017

Looking good so far.

@BridgeAR
Copy link
Member

BridgeAR commented Oct 2, 2017

Ping @strugee

@strugee
Copy link
Contributor Author

strugee commented Oct 13, 2017

Just an update, this is still on my radar but I was on break this past weekend and had a bunch of midterm papers due the week before so I'm pretty behind on all my technical work :P

Hopefully will have time soon... not sure when though 👍

@jasnell
Copy link
Member

jasnell commented Oct 13, 2017

@strugee ... sounds good. We'll keep this open

@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Oct 13, 2017
@BridgeAR
Copy link
Member

Ping @strugee

@strugee
Copy link
Contributor Author

strugee commented Dec 20, 2017

@BridgeAR hey, so sorry to leave this hanging open. I'm on break starting Friday; getting this to the finish line will be my first priority then 👍

@strugee
Copy link
Contributor Author

strugee commented Jan 16, 2018

At this point I think it's time to just admit I won't have time/motivation to finish this and close this PR. Sorry to keep it hanging open for so long, I feel pretty silly :(

If anyone wants to pick this up again I'd be happy to answer questions about the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. wip Issues and PRs that are still a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants