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
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
6572115
3850 - if you're using the corepack (https://nodejs.org/api/corepack.…
inoyakaigor Apr 2, 2024
eabcdde
3850 - first implementation of new Set methods
inoyakaigor Apr 2, 2024
9763039
3850 - call a method on the argument
inoyakaigor Apr 15, 2024
7131add
3850 - Add code to few another Set methods; align typings to TS PR#57230
inoyakaigor Apr 18, 2024
6b93c8a
3850 - review fixes: move reportObserved() into conditions branch; ch…
inoyakaigor Apr 18, 2024
4e438bb
3850 - add rest of the Set methods
inoyakaigor Apr 26, 2024
f5d6863
3850 - align functions parameter type declarations with its expected …
inoyakaigor May 2, 2024
46df92b
3850 - update TS to version with new Set methods; remove copied types…
inoyakaigor May 17, 2024
4a258c7
3850 - fix type for the «difference» function
inoyakaigor May 17, 2024
cf22937
3850 - add ESNext to tsconfig lib to fix tests
inoyakaigor May 17, 2024
a109642
3850 - update TS to Beta
inoyakaigor Jun 6, 2024
4767248
3850 - fix build failing (finally!)
inoyakaigor Jun 6, 2024
538ac5a
3850 - add tests for Set methods
inoyakaigor Jun 11, 2024
30fa622
3850 - change test for Set methods; change source code for methods wh…
inoyakaigor Jun 16, 2024
615b9e7
3850 - bump Nodejs version to 22 because it is support new Set method…
inoyakaigor Jun 16, 2024
68887de
Merge branch 'master' into 3850-new-set-methods
inoyakaigor Jun 16, 2024
7b7317b
3850 – tests for set-like objects
inoyakaigor Jun 19, 2024
74a307a
3850 – tests for check reactivity
inoyakaigor Jun 19, 2024
8ca04e6
3850 – update deps to TS 5.5; remove a packageManager field from pack…
inoyakaigor Jun 25, 2024
5f06ab2
3850 – cancel installation.md changes
inoyakaigor Jun 25, 2024
00d4cc9
3850 – esnext already contains es6
inoyakaigor Jun 30, 2024
81c2329
3850 – change types for new methods and checks for otherSet is has Se…
inoyakaigor Jul 1, 2024
113558b
3850 – review fixes: clarify the types
inoyakaigor Jul 1, 2024
4acd882
3850 – review fixes: removed unnecessary code
inoyakaigor Jul 2, 2024
d68dd2a
Merge branch 'main' into 3850-new-set-methods
urugator Jul 2, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/build_and_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ jobs:
- name: Checkout Repo
uses: actions/checkout@master

- name: Setup Node.js 20.x
- name: Setup Node.js 22.x
uses: actions/setup-node@master
with:
node-version: 20.x
node-version: 22.x

- name: Install Dependencies
run: yarn --frozen-lockfile --ignore-scripts
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/coveralls.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ jobs:
# This makes Actions fetch all Git history so that Changesets can generate changelogs with the correct commits
fetch-depth: 0

- name: Setup Node.js 20.x
- name: Setup Node.js 22.x
uses: actions/setup-node@master
with:
node-version: 20.x
node-version: 22.x

- name: Install Dependencies
run: yarn --frozen-lockfile --ignore-scripts
Expand Down
2 changes: 1 addition & 1 deletion docs/installation.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ For verification insert this piece of code at the beginning of your sources (eg.
if (!new class { x }().hasOwnProperty('x')) throw new Error('Transpiler is not configured correctly');
```

Note that for Next.js you must [customize Babel](https://nextjs.org/docs/advanced-features/customizing-babel-config) instead of TypeScript, even if your project is set up to use TypeScript.
Note that for Next.js you must [customize Babel](https://nextjs.org/docs/advanced-features/customizing-babel-config) instead of TypeScript, even if your project is set up to use TypeScript.

## MobX on older JavaScript environments

Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
],
"resolutions": {
"jest": "^29.5.0",
"typescript": "^5.0.2",
"typescript": "^5.5.2",
"recast": "^0.23.1"
},
"repository": {
Expand Down Expand Up @@ -68,7 +68,7 @@
"tape": "^5.0.1",
"ts-jest": "^29.0.5",
"tsdx": "^0.14.1",
"typescript": "^5.0.2"
"typescript": "^5.5.2"
},
"husky": {
"hooks": {
Expand Down
10 changes: 5 additions & 5 deletions packages/mobx/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1396,7 +1396,7 @@ A deprecation message will now be printed if creating computed properties while

```javascript
const x = observable({
computedProp: function() {
computedProp: function () {
return someComputation
}
})
Expand All @@ -1421,7 +1421,7 @@ or alternatively:

```javascript
observable({
computedProp: computed(function() {
computedProp: computed(function () {
return someComputation
})
})
Expand All @@ -1439,7 +1439,7 @@ N.B. If you want to introduce actions on an observable that modify its state, us
```javascript
observable({
counter: 0,
increment: action(function() {
increment: action(function () {
this.counter++
})
})
Expand Down Expand Up @@ -1565,10 +1565,10 @@ function Square() {
extendObservable(this, {
length: 2,
squared: computed(
function() {
function () {
return this.squared * this.squared
},
function(surfaceSize) {
function (surfaceSize) {
this.length = Math.sqrt(surfaceSize)
}
)
Expand Down
139 changes: 139 additions & 0 deletions packages/mobx/__tests__/v5/base/set.js
Original file line number Diff line number Diff line change
Expand Up @@ -289,3 +289,142 @@ test("set.forEach is reactive", () => {
s.add(2)
expect(c).toBe(3)
})

describe("The Set object methods do what they are supposed to do", () => {
const reactiveSet = set([1, 2, 3, 4, 5])

test("Observable Set methods returns correct result", () => {
const intersectionObservableResult = reactiveSet.intersection(new Set([1, 2, 6]))
const unionObservableResult = reactiveSet.union(new Set([1, 2, 6]))
const differenceObservableResult = reactiveSet.difference(new Set([1, 2, 3, 4, 5, 6, 7]))
const symmetricDifferenceObservableResult = reactiveSet.symmetricDifference(new Set([3, 4]))
const isSubsetOfObservableResult = reactiveSet.isSubsetOf(new Set([1, 2, 3]))
const isSupersetOfObservableResult = reactiveSet.isSupersetOf(new Set([1, 2, 3, 4, 5, 6]))
const isDisjointFromObservableResult = reactiveSet.isDisjointFrom(new Set([6, 7]))

expect(intersectionObservableResult).toEqual(new Set([1, 2]))
expect(unionObservableResult).toEqual(new Set([1, 2, 3, 4, 5, 6]))
expect(differenceObservableResult).toEqual(new Set())
expect(symmetricDifferenceObservableResult).toEqual(new Set([1, 2, 5]))
expect(isSubsetOfObservableResult).toBeFalsy()
expect(isSupersetOfObservableResult).toBeFalsy()
expect(isDisjointFromObservableResult).toBeTruthy()
})

test("Observable Set proper works with Set-like objects", () => {
const intersectionObservableResult = reactiveSet.intersection(
new Map([1, 2, 6].map(i => [i, i]))
)
const unionObservableResult = reactiveSet.union(new Map([1, 2, 6].map(i => [i, i])))
const differenceObservableResult = reactiveSet.difference(
new Map([1, 2, 3, 4, 5, 6, 7].map(i => [i, i]))
)
const symmetricDifferenceObservableResult = reactiveSet.symmetricDifference(
new Map([3, 4].map(i => [i, i]))
)
const isSubsetOfObservableResult = reactiveSet.isSubsetOf(
new Map([1, 2, 3].map(i => [i, i]))
)
const isSupersetOfObservableResult = reactiveSet.isSupersetOf(
new Map([1, 2, 3, 4, 5, 6].map(i => [i, i]))
)
const isDisjointFromObservableResult = reactiveSet.isDisjointFrom(
new Map([6, 7].map(i => [i, i]))
)

expect(intersectionObservableResult).toEqual(new Set([1, 2]))
expect(unionObservableResult).toEqual(new Set([1, 2, 3, 4, 5, 6]))
expect(differenceObservableResult).toEqual(new Set())
expect(symmetricDifferenceObservableResult).toEqual(new Set([1, 2, 5]))
expect(isSubsetOfObservableResult).toBeFalsy()
expect(isSupersetOfObservableResult).toBeFalsy()
expect(isDisjointFromObservableResult).toBeTruthy()
})
})

describe("Observable Set methods are reactive", () => {
let c = 0
let s = set()

beforeEach(() => {
c = 0
s = set()
})

test("Intersection method is reactive", () => {
autorun(() => {
s.intersection(new Set())
c++
})

s.add(1)
s.add(2)
expect(c).toBe(3)
})

test("Union method is reactive", () => {
autorun(() => {
s.union(new Set())
c++
})

s.add(1)
s.add(2)
expect(c).toBe(3)
})

test("Difference method is reactive", () => {
autorun(() => {
s.difference(new Set())
c++
})

s.add(1)
s.add(2)
expect(c).toBe(3)
})

test("symmetricDifference method is reactive", () => {
autorun(() => {
s.symmetricDifference(new Set())
c++
})

s.add(1)
s.add(2)
expect(c).toBe(3)
})

test("isSubsetOf method is reactive", () => {
autorun(() => {
s.isSubsetOf(new Set())
c++
})

s.add(1)
s.add(2)
expect(c).toBe(3)
})

test("isSupersetOf method is reactive", () => {
autorun(() => {
s.isSupersetOf(new Set())
c++
})

s.add(1)
s.add(2)
expect(c).toBe(3)
})

test("isDisjointFrom method is reactive", () => {
autorun(() => {
s.isDisjointFrom(new Set())
c++
})

s.add(1)
s.add(2)
expect(c).toBe(3)
})
})
52 changes: 52 additions & 0 deletions packages/mobx/src/types/observableset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,58 @@ export class ObservableSet<T = any> implements Set<T>, IInterceptable<ISetWillCh
} as any)
}

intersection<U>(otherSet: ReadonlySetLike<U> & Partial<Set<U>>): Set<T & U> {
inoyakaigor marked this conversation as resolved.
Show resolved Hide resolved
if (typeof otherSet.intersection === "function") {
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

} else {
const dehancedSet = new Set(this)
return dehancedSet.intersection(otherSet)
}
}

union<U>(otherSet: ReadonlySetLike<U> & Partial<Set<U>>): Set<T | U> {
if (typeof otherSet.union === "function") {
this.atom_.reportObserved()
return otherSet.union(this.data_)
} else {
const dehancedSet = new Set(this)
return dehancedSet.union(otherSet)
}
}

difference<U>(otherSet: ReadonlySetLike<U> & Partial<Set<T>>): Set<T> {
inoyakaigor marked this conversation as resolved.
Show resolved Hide resolved
return new Set(this).difference(otherSet)
}

symmetricDifference<U>(otherSet: ReadonlySetLike<U> & Partial<Set<U>>): Set<T | U> {
if (typeof otherSet.symmetricDifference === "function") {
this.atom_.reportObserved()
return otherSet.symmetricDifference(this.data_)
} else {
const dehancedSet = new Set(this)
return dehancedSet.symmetricDifference(otherSet)
}
}

isSubsetOf(otherSet: ReadonlySetLike<unknown> & Partial<Set<unknown>>): boolean {
inoyakaigor marked this conversation as resolved.
Show resolved Hide resolved
return new Set(this).isSubsetOf(otherSet)
}

isSupersetOf(otherSet: ReadonlySetLike<unknown> & Partial<Set<unknown>>): boolean {
inoyakaigor marked this conversation as resolved.
Show resolved Hide resolved
return new Set(this).isSupersetOf(otherSet)
}

isDisjointFrom(otherSet: ReadonlySetLike<unknown> & Partial<Set<unknown>>): boolean {
if (typeof otherSet.isDisjointFrom === "function") {
this.atom_.reportObserved()
return otherSet.isDisjointFrom(this.data_)
} else {
const dehancedSet = new Set(this)
return dehancedSet.isDisjointFrom(otherSet)
}
}

replace(other: ObservableSet<T> | IObservableSetInitialValues<T>): ObservableSet<T> {
if (isObservableSet(other)) {
other = new Set(other)
Expand Down
2 changes: 1 addition & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"target": "es6",
"module": "esnext",
"moduleResolution": "node",
"lib": ["es6"],
"lib": ["esnext"],
"downlevelIteration": true,
"alwaysStrict": true,
"sourceMap": true,
Expand Down
Loading