Supporting a new set methods#3853
Conversation
|
|
@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 |
|
Also there is a small change in the |
I think the idea was something like
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.
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? class ObservableSet<T = any> implements Set<T>, IInterceptable<ISetWillChange>, IListenable {
[$mobx] = ObservableSetMarker
private data_: Set<T /* ← here */> = new Set()
// rest of code |
No, it's supposed to return new
I don't understand the question.
I think it needs to be
Afaik we don't have to do anything to support these. |
I think this methods should trigger |
|
Don't forget to provide tests please. We want to test 2 things:
Feel free to write tests for these as well and we will decide how to proceed based on the results 😉. |
…ange strict inequality operator to strict equality
inoyakaigor
force-pushed
the
3850-new-set-methods
branch
from
006c43b to
4767248
Compare
…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
| intersection<U>(otherSet: ReadonlySetLike<U> | Set<U>): Set<T & U> { | ||
| if (isES6Set(otherSet)) { | ||
| this.atom_.reportObserved() | ||
| return otherSet.intersection(this.data_) |
There was a problem hiding this comment.
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 {
// ...
}There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Oh, I've tought you're mentioned it in your latest messages. Totally forgot about this one! In this case everything is okay
|
Thank you. However we forgot the changeset... can you provide it in another PR? |
|
@urugator it's because of Node's version |
|
Is there another place where I have to change Node's version instead of UPD: Also |
3 participants
Code change checklist
/docs. For new functionality, at leastAPI.mdshould be updatedyarn 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)