tc39 / proposal-collection-methods

https://tc39.github.io/proposal-collection-methods/
Other
172 stars 8 forks source link

Construction and addition order #32

Closed Ginden closed 3 months ago

Ginden commented 5 years ago

.filter and similar methods can either:

  1. Create new instance, perform algorithm on internal data structure, and then use AddEntriesFromIterable.
  2. Perform algorithm on internal data structure, and then create new instance (it will internally use AddEntriesFromIterable)

This can be observed by user in few twisted cases. Eg.

class FooSet extends Set {
  add(...args) { 
      console.log('foo');
      return super.add(...args);
   }
}

(new FooSet([1])).filter(() => console.log('bar'));

This code, depending on our choice, will either log foo,bar or bar,foo.

Personally I'm in favor of first option.

ljharb commented 5 years ago

Because the constructor calls add, it would have to log foo,foo,bar or foo,bar,foo

zloirock commented 5 years ago

@ljharb console.log returns undefined -> .add will be called once on new FooSet([1]), but not once on result set. Why do you expect 2 foo?

ljharb commented 5 years ago

Because filter calls add by constructing a new set? But you’re right that bar wouldn’t be added anywhere because the predicate returns undefined

zloirock commented 5 years ago

Empty set.

zloirock commented 5 years ago

For

class FooSet extends Set {
  add(...args) { 
      console.log('foo');
      return super.add(...args);
   }
}

(new FooSet([1])).filter(() => console.log('bar') || true);

should be foo,bar,foo.

zloirock commented 5 years ago

More interesting this case:

class FooSet extends Set {
  add(...args) { 
      console.log('foo');
      return super.add(...args);
   }
}

(new FooSet([1, 2])).filter(() => console.log('bar') || true);

With AddEntriesFromIterable we will have foo,foo,bar,bar,foo,foo. But for me seems more logical foo,foo,bar,foo,bar,foo and direct adding of value after each calling of callback.

Ginden commented 5 years ago

Working example:

class FooSet extends Set {
   constructor(...args) {
      console.log('ctr');
      super(...args);
   }
   add(val) {
       console.log('add');
       return super.add(val);
    }
}

(new FooSet([1, 2])).filter(() => {
   console.log('predicate');
   return true;
});

This will always print ctr => add => add first (because of (new FooSet([1, 2]))).

Then it can print one of these:

  1. ctr => predicate => add => predicate => add ("create set and add element after each call to predicate")
  2. predicate => predicate => ctr => add => add ("use internal list, then create set from that list")
  3. ctr => predicate => predicate => add => add ("create set, operate on internal list, then add elements to set")

Array use approach 1, but it isn't observable to user unless Proxy with defineProperty trap is returned from FooArray subclass (gist). Therefore, I'm in favor of 1.

@zloirock @ljrharb Can I hide your comments to make thread clearer to readers?

Ginden commented 3 months ago

Given precedence set by new Set methods and Map.groupBy, it's clear that committee wants to prevent users from monkey-patching builtin objects.