⚠️ No Changeset found

Latest commit: f5d6863

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mweststrate at the issue comment you've mentioned that it should be enough to call an original Set.prototype method so I've copypasted a code from a Observableset.values() method. Is it correct? Should I add / remove / rewrite something?

Also there is a small change in the package.json. This field was added by a Node.js's manager of package managers Corepack. It is because I've ran into a problem at initial setup the project because for developing I have to use yarn@1 but in my daily job I'm using yarn@4. It isn't a breaking change.

@mweststrate at the #3850 (comment) you've mentioned that it should be enough to call an original Set.prototype method

I think the idea was something like Set.prototype.intersection.call(observaleSetOrJustIterator, otherSet), but I suspect it won't be possible:

intersection() accepts set-like objects as the other parameter. It requires this to be an actual Set instance, because it directly retrieves the underlying data stored in this without invoking any user code.

The set-like protocol invokes the keys() method instead of [@@iterator](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set/@@iterator) to produce elements. This is to make maps valid set-like objects, because for maps, the iterator produces entries but the has() method takes keys.

So you will probably have to create intermediate Set with dehanced values and intersect it instead.

If the argument is actual Set (instead of just being set-like) and if the operation is commutative (like intersection), it can be optimized by calling the operation on the argument:

intersection(otherSet: SetLike<T>): SetLike<T> {
   if (typeof otherSet.intersection === 'function') {
      return otherSet.intersection(this);
   } else {
      const dehancedSet = new Set(this);
      return dehancedSet.intersection(otherSet);
   }   
}

@urugator I've just fixed the code of intersetion method according to your comment.
I have few questions:

1. Should I keep an makeIterable code in the new functions? I mean this one:

return makeIterable<T & U>({
            next() {
                return nextIndex < observableValues.length
                    ? { value: self.dehanceValue_(observableValues[nextIndex++]), done: false }
                    : { done: true }
            }
        } as any)

or it is better write as in your comment?
Also am I understands correctly that code delaying possible side effects as maximum as posssible when outer code reading the ObservableSet values?
2) (non -relative to the PR) Wouldn't it be better to change a typing for a private data_ field to the same generic type as in class declaration?

class ObservableSet<T = any> implements Set<T>, IInterceptable<ISetWillChange>, IListenable {
    [$mobx] = ObservableSetMarker
    private data_: Set<T /* ← here */> = new Set()
    // rest of code

Should I keep an makeIterable code in the new functions? I mean this one:

No, it's supposed to return new Set, not an iterator.
Also const dehancedSet = new Set(this); already invokes the iterator.

Also am I understands correctly that code delaying possible side effects as maximum as posssible when outer code reading the ObservableSet values?

I don't understand the question.

Wouldn't it be better to change a typing for a private data_ field to the same generic type as in class declaration

I think it needs to be any because of the enhancer possibly changing the type.

Also I just realised that there also should be added Object.groupBy and Map.groupBy

Afaik we don't have to do anything to support these.

Afaik we don't have to do anything to support these.

I think this methods should trigger reportObserved() method at least

Don't forget to provide tests please. We want to test 2 things:

1. It does what it should. Use native Map for set-like arguments. Check results of our (observable) method calls against results of native (non-observable) method calls.
2. It's reactive.

I think this methods should trigger reportObserved() method at least

Feel free to write tests for these as well and we will decide how to proceed based on the results 😉.