feat(collections): add filterNotNullish

[majidsajadi]

This PR adds filterNotNullish to the collections module

[LionC]

I left some comments - the major one being that we should use a builtin internally here, as it does not create additional overhead.

[LionC]

I think we can be clear here and say "are neither null or undefined" to be very clear.

[majidsajadi]

@LionC

Returns all elements in the given collection that are neither null nor undefined

Is it okay?

[LionC]

Yes 👌

[LionC]

Same here

[LionC]

We have settled on array for these arguments in all the other functions, we should do the same here:

Suggested change

	collection: readonly T[],
	array: readonly T[],

[LionC]

I think simply using builtin filter will likely be faster because of potential magic v8 optimizations and it will not have any additional memory overhead in this case:

Suggested change

	const result: NonNullable<T>[] = [];
	for (const current of collection) {
	if (current !== undefined && current !== null) {
	result.push(current as NonNullable<T>);
	}
	}
	return result;
	return array.filter((it) => it !== undefined && it !== null);

[majidsajadi]

@LionC since we used for..of in every function I wanted to keep it consistent. so I should use built-in functions whenever it makes sense?

[LionC]

In the other functions, we use for..of to optimise for performance overhead, since the builtins like .map and .filter iterate through he whole array and create a copy, when it could be implemented more efficiently.

Here, however, the runtime complexity and memory overhead are exactly the same, so using the builtin is fine and will likely be faster because of optimizations.

[LionC]

I want to note that there has been discussion surrounding this function in the initial proposal because people felt like it is too simple.

However, I want to add, that almost everyone who complained about it in the initial discussion proposed a "trivial" replacement for it in the form of .filter((it) => it) which is not the same and will cause runtime bugs for number, string and boolean because of falseiness. To me, that shows that even experienced people fall into this trap when "just doing this quickly", which happens more often than one might think.

This implementation also gets types right, which typescript does not even for .filter((it) => it !== undefined && it !== null), because it does not detect that condition as a typeguard. So if you do not want to use filterNotNullish, you have to write very verbose and very ugly code to fix the typings.

Also, same as always, kotlin has it and it is being used a lot, yada yada^^

I want to make sure that discussion around this happens properly, so be prepared for this being PR being open for a moment :-)

[majidsajadi]

@LionC updated.

[LionC]

LGTM, pinging @kt3k

[lucacasonato]

Hey @majidsajadi. Thanks for the PR.

I am not going to merge this PR, because I think we are starting to scope creep std/collections here. This PR adds nearly 100 lines of code we need to maintain. We have removed a lot more complex functions from std because of triviality (e.g. denoland/deno#7256).

This is a 1 line function that takes about 30 seconds to write by hand. Going to deno.land/std and finding this function to import takes significantly longer than 30 seconds.

Some more background why trivial functions like these should be written by hand for each scenario:

• What if I want to filter only undefined (not null)?
• What if I want to filter undefined + numbers?
• What if I want to filter all falsy values?

Merging this PR sets the precedent that trivial functions like ones described above would be accepted into std.