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

BREAKING(std/collections): remove distinct and union #1099

Closed
wants to merge 1 commit into from
Closed

BREAKING(std/collections): remove distinct and union #1099

wants to merge 1 commit into from

Conversation

timreichen
Copy link
Contributor

distinct and union are sugar functions for trivial one-liners

distinct

Array.from(new Set(array))

union

Array.from(new Set(arrays.flat()))

and should therefore be removed from std/collections

@timreichen timreichen changed the title (std/collections): remove distinct and union BREAKING(std/collections): remove distinct and union Aug 5, 2021
@grian32
Copy link
Contributor

grian32 commented Aug 5, 2021

I disagree with this change:

  • The implementations are not so trivial to beginners
  • Makes code more readable
  • Less room for bugs/easier to find them(1 function call thats self-explanatory over 2/3)
  • It doesn't really cause any issues(see kotlin stdlib, which this was based off of)
  • If any new optimizations are found the user doesn't have to update his own implementation, they just have to update their dependencies.

@timreichen
Copy link
Contributor Author

I disagree with this change:

  • The implementations are not so trivial to beginners

This is a common pattern for distinct values.

  • Makes code more readable

Disagree. Array.from(new Set(array)) is more readable imo because the trivial functionality is not hidden under a function.

  • Less room for bugs/easier to find them(1 function call thats self-explanatory over 2/3)

It is sugar, so it is exactly the same functionality.

  • It doesn't really cause any issues(see kotlin stdlib, which this was based off of)

std should not implement sugar functions for trivial cases, therefore causing the issue of being a counter example.

  • If any new optimizations are found the user doesn't have to update his own implementation, they just have to update their dependencies.

not very likely, more likely that optimization is going to happen under the hood imo.

@grian32
Copy link
Contributor

grian32 commented Aug 5, 2021

This is a common pattern for distinct values.

Not to a beginner(not with Deno, with JS/TS in general), especially if we're trying to target them more (as seen in the controversial --no-check by default issue/pr)

Disagree. Array.from(new Set(array)) is more readable imo because the trivial functionality is not hidden under a function.

I think you misunderstood what I was trying to say, seeing distinct(arr) is more readable than Array.from(new Set(arr)) because it's literally just telling you that it gets the unique values, you don't have to process that its making a new set the converting it to array etc.

It is sugar, so it is exactly the same functionality.

Again, I think you misunderstood, it's much easier to accidentally forget to .flat() an array if youre typing this 20 times or something along those lines, that isn't really a possibility with these functions.

std should not implement sugar functions for trivial cases, therefore causing the issue of being a counter example.

Not sure what you mean by "being a counter example" in this context could you elaborate?

not very likely, more likely that optimization is going to happen under the hood imo.

/shrug js could get new features that make this more optimize-able, the possibility is there and worth accounting for.

@LionC
Copy link
Contributor

LionC commented Aug 5, 2021

There has been a lot of discussion around the collection functions. Without going into the specifics of the two mentioned here regarding optimization, expressiveness etc., I find it hard to see the value generated by simply removing functions from a module because you personally would not use them. They are utility functions inspired by heavily used libraries. Not everyone will use them, they will not add value for everyone and there might even be people who hate them. That is why the module is written in a way that you can only include the ones you want to use.

I also want to add that while I understand the idea, a PR that just removes things that have just been merged as the result of a long process with discussions, reviews and several people, leaves a weird aftertaste with me personally.

@zhangyuannie
Copy link
Contributor

I do not think it is right to remove them with a simple PR without discussions. However, I think it's still worth revisiting if they should stay in std. Here are some points that I think are very valid:

1. They abstract simple one-liners

const uniques = [...new Set(arr)];

is a very common pattern to get unique values out of an array, and I'll even argue that this is even more elegant and readable than distinct(arr). On top of that, it works with all iterables, not just arrays.

While it is debatable, but the general trend in js is to let these trivial abstractions created before ES6 go and to use their ES6 counterpart [1][2]. Including them in std seems like a step backward.

2. This does not seem to align with the general direction of std-wg

While there is no clear consensus on the scope of std, the general idea that has surfaced is for std to be minimal and un-opinionated. More elaborated one-liners than these have been removed from std before: #733 #728.

People can use something like https://deno.land/x/[email protected] if they really want these helpers.

3. We should not do stuff that might be deprecated in the future

There is already a stage 2 proposal for union for Set: https://github.com/tc39/proposal-set-methods, while we are dealing with arrays here, the conversion between the two is quite trivial.

tldr

I think there's a case to be made and we should discuss it further.

@grian32
Copy link
Contributor

grian32 commented Aug 10, 2021

People can use something like https://deno.land/x/[email protected] if they really want these helpers.

You can't import the published lodash from /x/(kinda weird that it's still there but it is what it is), you have to import it from skypack or something similar.

is a very common pattern to get unique values out of an array,

It's not really that obvious to a beginner imho(and as seen by the remove type-checking by default, we're attempting to target beginners).

You go and look up "how to get all the unique values in an array javascript" first post you get is: https://stackoverflow.com/q/1960473/14560865, which the accepted answer has some non-modern versions, with this pattern at the very bottom of it, the original question also links to: https://stackoverflow.com/q/9229645/14560865 and https://stackoverflow.com/q/1960473/14560865, one of which is not very modern and one which uses jquery.

There is already a stage 2 proposal for union for Set: https://github.com/tc39/proposal-set-methods, while we are dealing with arrays here, the conversion between the two is quite trivial.

Could say the same thing about the entirety of std/collections considering iterator helpers proposal exist, but it's still here so i think removing stuff based on proposals that will take years to get implemented is kindof a weird take.

And removing stuff when they don't really hurt anyone and has multiple benefits to their existence(see my earlier comment) because they're easily available in 3rd party modules or there's a proposal for it leaves std in a really weird position, because you could make the same case for some parts of std(std/fmt/colors.ts for example because its just color codes and probably already exists in third party on skypack.dev or esm.sh)

@LionC
Copy link
Contributor

LionC commented Aug 10, 2021

@zhangyuannie sorry this got really long - I am just repeating myself in different channels a lot in very similir discussions and wanted to be a bit more thorough to be able to link to this. I hope you are not feeling like I am ranting at you :-)

There has been a long discussion in discord about this, see this message and everything above it . It touches a lot of the points.

The biggest problem here is that many people seem to have strong-ish conceptions about the philosophy behind std without there being an explicit point of reference. This message implies that there is no established minimalism goal with std - which makes sense for me because the goal also seems tl be to give people a standard toolset for common cases so we do not repeat the npm madness.

In the end, I think the discussion behind this is a lot more about general std philosophy than about the functions at hand at it might be a good idea to centralize that instead of having anecdotal discussions that are actually about something else.

For the functions, I can just repeat what I said in the discussion leading to the first message above.

To reiterate:

  • std is not the ES spec. I would argue that the whole point of std (and by some extent every module) is to provide things that the spec does not want to include. There is also not really any common code style in JS/TS, the languages are very style agnostic and for each style of doing things, there are likely millions of developers in the ecosystem believing in it. std will mor change tat, but it will give everyone more tools to succeed without trusting third parties.
  • There is significant evidence in kotlin/std/collections and lodash that people appreciate methods like this for multiple reasons (readability/expressiveness, blackboxing etc)
  • I do not see the harm caused in having those methods, but there is gain for people who like using them. Most of the discussions I am having about this module seem to boil down to "I would not add those methods myself / I would not use them / I am not sure if they are worth the time", which is all not really an issue if we have (several) people who are willing to do it and have already done the majority of work with core members and if people are free to choose what to use and what not to use (std is a highly modularized external dependency after all), both of which I think to be true.
  • I do not see things existing in userland or being possible in userland as an argument for or against something being in std. Everything could be done in
    userland and everything could be done in std, if everything would be be done in userland, std would not exists and vice versa.

@wperron
Copy link
Contributor

wperron commented Aug 10, 2021

While I'm sympathetic to the argument made by @timreichen, these functions have been discussed at length, both in the proposal and in Discord, and I think they bring a semantic value that outweighs the fact that the implementation is so simple. For now we'll keep them in the std lib.

@wperron wperron closed this Aug 10, 2021
@timreichen timreichen deleted the std/collections/remove-distinct-union branch November 15, 2021 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants