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

Supporting a new set methods #3853

Merged
merged 25 commits into from
Jul 2, 2024
Merged

Conversation

inoyakaigor
Copy link
Contributor

@inoyakaigor inoyakaigor commented Apr 2, 2024

Code change checklist

  • Added/updated unit tests
  • (Not applicable) Updated /docs. For new functionality, at least API.md should be updated
  • Verified that there is no significant performance drop (yarn mobx test:performance)

Fixes #3850

These changes depends on microsoft/TypeScript/pull/57230 . According TS iteration plan (microsoft/TypeScript/issues/57475) it should be landed in TS 5.5 (in June'24)

Copy link

changeset-bot bot commented Apr 2, 2024

⚠️ No Changeset found

Latest commit: d68dd2a

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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

@inoyakaigor
Copy link
Contributor Author

@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?

@inoyakaigor
Copy link
Contributor Author

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.

@urugator
Copy link
Collaborator

urugator commented Apr 3, 2024

@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.

@urugator
Copy link
Collaborator

urugator commented Apr 3, 2024

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);
   }   
}

@inoyakaigor
Copy link
Contributor Author

@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

@urugator
Copy link
Collaborator

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.

@inoyakaigor
Copy link
Contributor Author

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

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

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@urugator
Copy link
Collaborator

urugator commented Apr 18, 2024

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 😉.

…ich gives different results depends on place of operands:

SET1 AGAINST SET2
  intersection         Set(2) { 1, 2 }
  union                Set(6) { 1, 2, 3, 4, 5, 6 }
  difference           Set(2) { 4, 5 }
  symmetricDifference  Set(3) { 1, 2, 5 }
  isSubsetOf           false
  isSupersetOf         false
  isDisjointFrom       true

SET2 AGAINST SET1
  intersection         Set(2) { 1, 2 }
  union                Set(6) { 1, 2, 6, 3, 4, 5 }
  difference           Set(0) {}
  symmetricDifference  Set(3) { 1, 2, 5 }
  isSubsetOf           true
  isSupersetOf         true
  isDisjointFrom       true
tsconfig.json Outdated Show resolved Hide resolved
intersection<U>(otherSet: ReadonlySetLike<U> | Set<U>): Set<T & U> {
if (isES6Set(otherSet)) {
this.atom_.reportObserved()
return otherSet.intersection(this.data_)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't correct, it won't work with custom enhancer. Was there any problem with the code I've proposed originally? That is:

if (isES6Set(otherSet)) {
   return otherSet.intersection(this)
} else {
  // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked twice where you mentioned about deleting reportObserved() but didn't find. Maybe you're assumed this but I didn't get it. So anyway removing the reportObserved() throwing errors:

image

Copy link
Collaborator

@urugator urugator Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where you mentioned about deleting reportObserved() but didn't find.

I didn't mention deleting reportObserved anywhere, because I didn't propose adding it in the first place:
#3853 (comment)

So anyway removing the reportObserved() throwing errors:

Did you just removed reportObserved or did you also replaced this._data with this as suggested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I've tought you're mentioned it in your latest messages. Totally forgot about this one! In this case everything is okay

@urugator urugator merged commit 3efb345 into mobxjs:main Jul 2, 2024
1 check passed
@inoyakaigor inoyakaigor deleted the 3850-new-set-methods branch July 2, 2024 12:36
@inoyakaigor inoyakaigor restored the 3850-new-set-methods branch July 2, 2024 12:37
@urugator
Copy link
Collaborator

urugator commented Jul 2, 2024

Thank you. However we forgot the changeset... can you provide it in another PR? yarn changeset

@urugator
Copy link
Collaborator

urugator commented Jul 2, 2024

@inoyakaigor
Copy link
Contributor Author

@urugator it's because of Node's version
image
Only v22 (maybe also v21 I didn't check) supports Set methods

@inoyakaigor
Copy link
Contributor Author

inoyakaigor commented Jul 2, 2024

Is there another place where I have to change Node's version instead of .github/workflows/coveralls.yml and .github/workflows/build_and_test.yml?

UPD: Also .github\workflows\release.yml have to be changed

@inoyakaigor inoyakaigor mentioned this pull request Jul 2, 2024
3 tasks
@inoyakaigor
Copy link
Contributor Author

@urugator I've made new PR #3898

urugator pushed a commit that referenced this pull request Jul 2, 2024
taj-p pushed a commit to taj-p/mobx that referenced this pull request Aug 30, 2024
taj-p pushed a commit to taj-p/mobx that referenced this pull request Aug 30, 2024
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.

Supporting of new Set methods in observable Sets
3 participants