docs: stronger suggestion for userland assert

[geek]

Fixes #4532

Credit to @mikeal for language, see #4532 (comment)

[evanlucas]

LGTM

[sam-github]

@mikeal This statement should not be this strong. There is no commonly npm package in existence for asserting _invariants_ in source. At least, not that I know of, can you point to one?

I suggest we make clear that assert is NOT for unit tests, and not point people to non-existent modules for the use-case that assert is a good tool for (invariants).

The assert module provides a simple set of assertion tests that can be used
to test invariants. While the assert module is generally intended for internal use by Node.js itself, it can be used by user code calling require('assert'). For the purposes of unit testing, it is recommended that users of Node.js choose a third party test library from [npm][] instead of relying on assert.

[sam-github]

OK, I see, searching for assert on npmjs.org gets you pages of results, none of which are useful to replace assert for invariants, and none of which are a unit test framework (which is probably what the reader wanted).

The usage of assert we want to warn people away from is using it as a unit testing library, such as mocha, tap, and nunit, and none of those are even in the first page of search results when using npmjs.org search.

Of course, I can't find a search term (other than "mocha") that yields mocha in the first page. :-( This is likely because npmjs.org search is unranked, so not so helpful, googling for node test framework works better.

[jmm]

@sam-github I'm a bit confused by your comments and feel that I must be misunderstanding something, so perhaps you could help me understand by rephrasing?

This statement should not be this strong. There is no commonly npm package in existence for asserting _invariants_ in source. At least, not that I know of, can you point to one?
...
OK, I see, searching for assert on npmjs.org gets you pages of results, none of which are useful to replace assert for invariants

Isn't assert, for example, pretty much the same thing? That's the first result I get when I search assert.

FWIW, there's also invariant which is dead simple (basically just like assert()).

and none of which are a unit test framework (which is probably what the reader wanted).

The usage of assert we want to warn people away from is using it as a unit testing library, such as mocha, tap, and nunit,

Do people somehow confuse the assert module as being a testing framework or library? Or do you mean it shouldn't be used for assertions in userland tests period? I think it's fairly commonly used for assertions within a framework like Mocha.

[jasnell]

LGTM
I know there's a lack of clarity on which third party assertion libraries to recommend (or even if there are any that are commonly used) but this change is fine, IMHO.

[sam-github]

I think Node.js has APIs that are legacy, and we should steer people clear of, but putting phrasing at the top of assert that basically implies "it sucks, we won't improve it because it is locked and is good enough for node, here's a link to an npmjs.org search query with pages of results... good luck" does a disservice to the assert API, which was intended as an analogue of the assert() we all know and love from C, not a unit testing library.

Sure, we should protect people from using assert for things it wasn't intended for, but we shouldn't imply that a perfectly usable Node.js API is to be avoided.

And to put it another way:

What is so special about assert that we are pointing people to npmjs.org and saying there are better things there?

If we are going to do that, then why don't we do it everywhere? Why doesn't https://nodejs.org/api/http.html come with a similar warning, telling people they should be looking on npmjs.org for request, or express/hapi/whatever instead of using the low-level node API? Or cluster, telling people to search npmjs.org for process manager?

Anyhow, I won't belabour this point further: while Node.js has some genuinely unfortunate APIs that should be avoided if not fixed, I don't think assert is one of them, so making assert out to be an API to be avoided does it a disservice.

/to @cjihrig, and @mikeal (who suggested this change in the first place, and should weigh in to why he thinks its helpful)

[geek]

@sam-github I am fine either way... I just want to make whatever the decision is clear. At the moment, the wording on https://nodejs.org/api/assert.html makes it sound like people should avoid using assert, if thats the case I want to make it clearer. If not, then we should remove the suggestion to avoid assert.

[cjihrig]

The future of assert came up in a CTC meeting. The notes aren't very detailed (could probably go back and listen to the audio), but the resolution was that assert should only be used for Node's internal testing. Internal modules didn't exist when assert was created, or it would have likely been an internal module.

IIRC a big issue was breaking changes in assert introduced by trying to keep up with new language features and edge cases (comparing Error objects, for example).

I completely agree with @sam-github that assert is not a module that needs to be avoided. I think the decision was more a reluctance of the core team to maintain an assertion library, which can be opinionated and easily implemented in userland.

Regarding this issue, I think we should:

• Remove the link out to npm search results.
• Mention that assert is meant for Node's own internal testing.
• Mention that it is definitely not a testing framework, and is not intended as a general purpose assertion library.

[sam-github]

I'd be good with @cjihrig's suggestions.

[cjihrig]

LGTM

[sam-github]

LGTM, thanks for humouring me on this.

[jasnell]

Minor nit... I'd like to avoid starting sentences with a lower-cased module name, especially two sentences in a row... Perhaps reword the las two sentences to:

The module is intended for internal use by Node.js, but can be used in application 
code via `require('assert')`. However, `assert` is not a testing framework, and is not 
intended to be used as a general purpose assertion library.

[jasnell]

Minor nit, otherwise LGTM

[mscdex]

Subsystem prefix should probably be doc: instead of docs: to be consistent with most other doc commits.

[silverwind]

LGTM

[cjihrig]

Landed in d7c5110