-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
base: main
Are you sure you want to change the base?
Conversation
…html) it enables correct version of Yarn
|
@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 |
}, | ||
"husky": { | ||
"hooks": { | ||
"pre-commit": "pretty-quick --staged" | ||
} | ||
} | ||
}, | ||
"packageManager": "[email protected]+sha256.732620bac8b1690d507274f025f3c6cfdc3627a84d9642e38a07452cc00e0f2e" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO remove before merge
package.json
Outdated
@@ -68,11 +68,12 @@ | |||
"tape": "^5.0.1", | |||
"ts-jest": "^29.0.5", | |||
"tsdx": "^0.14.1", | |||
"typescript": "^5.0.2" | |||
"typescript": "^5.5.0-dev.20240415" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO consolidate before merge
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
Code change checklist
/docs
. For new functionality, at leastAPI.md
should 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)