assert: add includes api

[zackschuster]

very simple proxy to StringPrototypeIncludes

inspired by #43784 (comment)

[zackschuster]

@aduh95

[cjihrig]

very simple proxy to StringPrototypeIncludes

My concern with adding APIs like this is that it is a slippery slope to a more confusing API. Right now this is a wrapper around StringPrototypeIncludes but people are likely to ask for this to support objects, Sets, arrays, etc. See the include() API in chai, @hapi/code, and probably other assertion libraries.

[mscdex]

I agree it isn't very worthwhile to add a (very specific wrapper) function like this.

[zackschuster]

My concern with adding APIs like this is that it is a slippery slope to a more confusing API. Right now this is a wrapper around StringPrototypeIncludes but people are likely to ask for this to support objects, Sets, arrays, etc. See the include() API in chai, @hapi/code, and probably other assertion libraries.

i think your concern about confusion is valid, but as the desire is for an api that delivers a useful AssertionError, i think we can resolve it with our wordings for the error messages (e.g. by saying things like the <target> (of type <array|string|etc>) does not include <value> – or however it's decided to be worded 😄).

[tniessen]

I think most users would expect the negation assert.doesNotInclude() to exist as well (based on the existing API). However, ...

My concern with adding APIs like this is that it is a slippery slope to a more confusing API. Right now this is a wrapper around StringPrototypeIncludes but people are likely to ask for this to support objects, Sets, arrays, etc.

It's also a slippery slope to bloating the assert module with many other small functions (e.g., , assert.startsWith(), assert.doesNotStartWith(), assert.endsWith(), assert.doesNotEndWith(), etc.).

i think your concern about confusion is valid, but as the desire is for an api that delivers a useful AssertionError, i think we can resolve it with our wordings for the error messages (e.g. by saying things like the <target> (of type <array|string|etc>) does not include <value> – or however it's decided to be worded 😄).

Unfortunately, adding types to such an API later can be considered a breaking change (see #41007 (comment), for example). In short: if this API is defined to reject non-string inputs, then later allowing non-string inputs weakens the assertion.

---

assert.match('foo bar baz', /bar/) might not produce pretty error messages and the RegExp might require escaping, but it's versatile.

[zackschuster]

I think most users would expect the negation assert.doesNotInclude() to exist as well (based on the existing API). However, It's also a slippery slope to bloating the assert module with many other small functions (e.g., , assert.startsWith(), assert.doesNotStartWith(), assert.endsWith(), assert.doesNotEndWith(), etc.).

if we end up making includes variadic then i think we actually have a strong argument against such requests, as those new methods don't have the same semantic overlap as includes.

i don't mind adding a negation.

Unfortunately, adding types to such an API later can be considered a breaking change (see #41007 (comment), for example). In short: if this API is defined to reject non-string inputs, then later allowing non-string inputs weakens the assertion.

agreed, it would need to be done now, as part of this pr.

[Mifrill]

I think, it would be nice to add a valid case with for regexp, like:

assert.includes(/\bpass\b/, 'It will pass');

@zackschuster WDYT?

[zackschuster]

that's just the assert.match api, but in reverse. i think that would be far more confusing than helpful.

[Mifrill]

okay, but for me, the word "includes" suitable to regexp matches. Well, with that point of view, it probably better to rename includes into strictIncludes, similar to toEqual and toStrictEqual

[aduh95]

"strict equality" refers to a real JS thing (https://tc39.es/ecma262/#sec-isstrictlyequal), "strict includes" does not. I don't think we should go with strictIncludes.

the word "includes" suitable to regexp matches

Arguably match is even more suitable.

[aduh95]

the RegExp might require escaping

That's the big issue, there is no built-in way to escape regexes. If there was, there would be no need for this specific API (nor doesNotInclude, startsWith, doesNotStartsWith, etc.).

[tniessen]

the RegExp might require escaping

That's the big issue, there is no built-in way to escape regexes. If there was, there would be no need for this specific API (nor doesNotInclude, startsWith, doesNotStartsWith, etc.).

That is not a Node.js issue (e.g., here is a discussion from 12 years ago), and a TC39 proposal exists to address it.

[aduh95]

I think it would be a nice to have, especially since the TC39 proposal seems stalled, but I can see the argument why we wouldn't add it to Node.js to keep our API simpler. Maybe @nodejs/test_runner has some thoughts?
We should probably either close this PR, or fix the linter issue and add the doesNotInclude util.

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[ljharb]

@aduh95 i have the proposal on the agenda for next week’s plenary; it’s not stalled.