Skip to content

Commit

Permalink
Merge branch 'fix-2326-v6' of https://github.com/urugator/mobx into u…
Browse files Browse the repository at this point in the history 
…rugator-fix-2326-v6. Also fixes #2379
  • Loading branch information
@mweststrate
mweststrate committed Jun 29, 2020
2 parents fcd15e0 + fec22b0 commit 4d1397b
Show file tree
Hide file tree
Showing 4 changed files with 254 additions and 138 deletions.
22 changes: 2 additions & 20 deletions notes.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,29 +104,11 @@ PHilosophy: one way to do things
- [ ] Breaking: killed IObservableObject interface.
- [ ] Breaking: sorting or reversing an array in an derivation will now throw rather than warn.
- [ ] Breaking: sorting or reversing an array in an actino will no sort or reverse the source array rather than shallow copy.
- Fixed #2326
- Fixed #2379

## NOTES

### New state changes model:

action

- enter batching,
- state updates allowed, only if not tracking -or- in effect

effect

- disable tracking + action

track

- enable tracking
- disable updates

Side effects like state changes are not allowed inside derivations. You can explicitly suppress this message by using 'effect', at your own risk.

State changes are not allowed outside actions.

## Blog

- mobx 6 is more backward compatible than its predecessor
Expand Down
276 changes: 158 additions & 118 deletions src/types/observablearray.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ const arrayTraps = {
export class ObservableArrayAdministration
implements IInterceptable<IArrayWillChange<any> | IArrayWillSplice<any>>, IListenable {
atom_: IAtom
values_: any[] = []
readonly values_: any[] = [] // this is the prop that gets proxied, so can't replace it!
interceptors_
changeListeners_
enhancer_: (newV: any, oldV: any | undefined) => any
Expand Down Expand Up @@ -229,14 +229,16 @@ export class ObservableArrayAdministration
return this.dehanceValues_(res)
}

spliceItemsIntoValues_(index, deleteCount, newItems: any[]): any[] {
spliceItemsIntoValues_(index: number, deleteCount: number, newItems: any[]): any[] {
if (newItems.length < MAX_SPLICE_SIZE) {
return this.values_.splice(index, deleteCount, ...newItems)
} else {
const res = this.values_.slice(index, index + deleteCount)
this.values_ = this.values_
.slice(0, index)
.concat(newItems, this.values_.slice(index + deleteCount))
let oldItems = this.values_.slice(index + deleteCount)
this.values_.length = index + newItems.length - deleteCount
for (let i = 0; i < newItems.length; i++) this.values_[index + i] = newItems[i]
for (let i = 0; i < oldItems.length; i++)
this.values_[index + newItems.length + i] = oldItems[i]
return res
}
}
Expand Down Expand Up @@ -351,128 +353,166 @@ export function createObservableArray<T>(

// eslint-disable-next-line
export var arrayExtensions = {
clear(): any[] {
return this.splice(0)
},

replace(newItems: any[]) {
const adm: ObservableArrayAdministration = this[$mobx]
return adm.spliceWithArray_(0, adm.values_.length, newItems)
},

// Used by JSON.stringify
toJSON(): any[] {
return this.slice()
},

/*
* functions that do alter the internal structure of the array, (based on lib.es6.d.ts)
* since these functions alter the inner structure of the array, the have side effects.
* Because the have side effects, they should not be used in computed function,
* and for that reason the do not call dependencyState.notifyObserved
*/
splice(index: number, deleteCount?: number, ...newItems: any[]): any[] {
const adm: ObservableArrayAdministration = this[$mobx]
switch (arguments.length) {
case 0:
return []
case 1:
return adm.spliceWithArray_(index)
case 2:
return adm.spliceWithArray_(index, deleteCount)
}
return adm.spliceWithArray_(index, deleteCount, newItems)
},
clear(): any[] {
return this.splice(0)
},

spliceWithArray(index: number, deleteCount?: number, newItems?: any[]): any[] {
return (this[$mobx] as ObservableArrayAdministration).spliceWithArray_(
index,
deleteCount,
newItems
)
},

push(...items: any[]): number {
const adm: ObservableArrayAdministration = this[$mobx]
adm.spliceWithArray_(adm.values_.length, 0, items)
return adm.values_.length
},

pop() {
return this.splice(Math.max(this[$mobx].values_.length - 1, 0), 1)[0]
},

shift() {
return this.splice(0, 1)[0]
},

unshift(...items: any[]): number {
const adm: ObservableArrayAdministration = this[$mobx]
adm.spliceWithArray_(0, 0, items)
return adm.values_.length
},

reverse(): any[] {
// reverse by default mutates in place before returning the result
// which makes it both a 'derivation' and a 'mutation'.
if (globalState.trackingDerivation) {
die(37, "reverse")
}
this.replace(this.slice().reverse())
return this
},

sort(): any[] {
// sort by default mutates in place before returning the result
// which goes against all good practices. Let's not change the array in place!
if (globalState.trackingDerivation) {
die(37, "sort")
}
const copy = this.slice()
copy.sort.apply(copy, arguments)
this.replace(copy)
return this
},

remove(value: any): boolean {
const adm: ObservableArrayAdministration = this[$mobx]
const idx = adm.dehanceValues_(adm.values_).indexOf(value)
if (idx > -1) {
this.splice(idx, 1)
return true
}
return false
replace(newItems: any[]) {
const adm: ObservableArrayAdministration = this[$mobx]
return adm.spliceWithArray_(0, adm.values_.length, newItems)
},

// Used by JSON.stringify
toJSON(): any[] {
return this.slice()
},

/*
* functions that do alter the internal structure of the array, (based on lib.es6.d.ts)
* since these functions alter the inner structure of the array, the have side effects.
* Because the have side effects, they should not be used in computed function,
* and for that reason the do not call dependencyState.notifyObserved
*/
splice(index: number, deleteCount?: number, ...newItems: any[]): any[] {
const adm: ObservableArrayAdministration = this[$mobx]
switch (arguments.length) {
case 0:
return []
case 1:
return adm.spliceWithArray_(index)
case 2:
return adm.spliceWithArray_(index, deleteCount)
}
return adm.spliceWithArray_(index, deleteCount, newItems)
},

spliceWithArray(index: number, deleteCount?: number, newItems?: any[]): any[] {
return (this[$mobx] as ObservableArrayAdministration).spliceWithArray_(
index,
deleteCount,
newItems
)
},

push(...items: any[]): number {
const adm: ObservableArrayAdministration = this[$mobx]
adm.spliceWithArray_(adm.values_.length, 0, items)
return adm.values_.length
},

pop() {
return this.splice(Math.max(this[$mobx].values_.length - 1, 0), 1)[0]
},

shift() {
return this.splice(0, 1)[0]
},

unshift(...items: any[]): number {
const adm: ObservableArrayAdministration = this[$mobx]
adm.spliceWithArray_(0, 0, items)
return adm.values_.length
},

reverse(): any[] {
// reverse by default mutates in place before returning the result
// which makes it both a 'derivation' and a 'mutation'.
if (globalState.trackingDerivation) {
die(37, "reverse")
}
this.replace(this.slice().reverse())
return this
},

sort(): any[] {
// sort by default mutates in place before returning the result
// which goes against all good practices. Let's not change the array in place!
if (globalState.trackingDerivation) {
die(37, "sort")
}
const copy = this.slice()
copy.sort.apply(copy, arguments)
this.replace(copy)
return this
},

remove(value: any): boolean {
const adm: ObservableArrayAdministration = this[$mobx]
const idx = adm.dehanceValues_(adm.values_).indexOf(value)
if (idx > -1) {
this.splice(idx, 1)
return true
}
return false
}
}

/**
* Wrap function from prototype
* Without this, everything works as well, but this works
* faster as everything works on unproxied values
*/
;[
"concat",
"every",
"filter",
"forEach",
"indexOf",
"join",
"lastIndexOf",
"map",
"reduce",
"reduceRight",
"slice",
"some",
"toString",
"toLocaleString"
].forEach(funcName => {
arrayExtensions[funcName] = function() {
/**
* Wrap function from prototype
* Without this, everything works as well, but this works
* faster as everything works on unproxied values
*/
addArrayExtension("concat", simpleFunc)
addArrayExtension("flat", simpleFunc)
addArrayExtension("includes", simpleFunc)
addArrayExtension("indexOf", simpleFunc)
addArrayExtension("join", simpleFunc)
addArrayExtension("lastIndexOf", simpleFunc)
addArrayExtension("slice", simpleFunc)
addArrayExtension("toString", simpleFunc)
addArrayExtension("toLocaleString", simpleFunc)
// map
addArrayExtension("every", mapLikeFunc)
addArrayExtension("filter", mapLikeFunc)
addArrayExtension("find", mapLikeFunc)
addArrayExtension("findIndex", mapLikeFunc)
addArrayExtension("flatMap", mapLikeFunc)
addArrayExtension("forEach", mapLikeFunc)
addArrayExtension("map", mapLikeFunc)
addArrayExtension("some", mapLikeFunc)
// reduce
addArrayExtension("reduce", reduceLikeFunc)
addArrayExtension("reduceRight", reduceLikeFunc)

function addArrayExtension(funcName, funcFactory) {
if (Array.prototype[funcName] === "function") {
arrayExtensions[funcName] = funcFactory(funcName)
}
}

// Report and delegate to dehanced array
function simpleFunc(funcName) {
return function() {
const adm: ObservableArrayAdministration = this[$mobx]
adm.atom_.reportObserved()
const res = adm.dehanceValues_(adm.values_)
return res[funcName].apply(res, arguments)
}
})
}

// Make sure callbacks recieve correct array arg #2326
function mapLikeFunc(funcName) {
return function(callback, thisArg) {
const adm: ObservableArrayAdministration = this[$mobx]
adm.atom_.reportObserved()
return adm.values_[funcName]((element, index) => {
element = adm.dehanceValue_(element)
return callback.call(thisArg, element, index, this)
})
}
}

// Make sure callbacks recieve correct array arg #2326
function reduceLikeFunc(funcName) {
return function(callback, initialValue) {
const adm: ObservableArrayAdministration = this[$mobx]
adm.atom_.reportObserved()
return adm.values_[funcName]((accumulator, currentValue, index) => {
currentValue = adm.dehanceValue_(currentValue)
return callback(accumulator, currentValue, index, this)
}, initialValue)
}
}

const isObservableArrayAdministration = createInstanceofPredicate(
"ObservableArrayAdministration",
Expand Down
47 changes: 47 additions & 0 deletions test/v4/base/array.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -570,3 +570,50 @@ test("reproduce #2021", () => {
delete Array.prototype.extension
}
})

test("correct array should be passed to callbacks #2326", () => {
const array = observable([1, 2, 3])

function callback() {
const lastArg = arguments[arguments.length - 1]
expect(lastArg).toBe(array)
}
;[
"every",
"filter",
"find",
"findIndex",
"flatMap",
"forEach",
"map",
"reduce",
"reduceRight",
"some"
].forEach(method => {
if (Array.prototype[method]) array[method](callback)
else console.warn("SKIPPING: " + method)
})
})

test("very long arrays can be safely passed to nativeArray.concat #2379", () => {
const nativeArray = ["a", "b"]
const longNativeArray = [...Array(10000).keys()]
const longObservableArray = observable(longNativeArray)
expect(longObservableArray.length).toBe(10000)
expect(longObservableArray).toEqual(longNativeArray)
expect(longObservableArray[9000]).toBe(longNativeArray[9000])
expect(longObservableArray[9999]).toBe(longNativeArray[9999])
expect(longObservableArray[10000]).toBe(longNativeArray[10000])

const expectedArray = nativeArray.concat(longNativeArray)
const actualArray = nativeArray.concat(longObservableArray.slice()) // NOTE: in MobX4 slice is needed

expect(actualArray).toEqual(expectedArray)

const anotherArray = [0, 1, 2, 3, 4, 5]
const observableArray = observable(anotherArray)
const r1 = anotherArray.splice(2, 2, ...longNativeArray)
const r2 = observableArray.splice(2, 2, ...longNativeArray)
expect(r2).toEqual(r1)
expect(observableArray).toEqual(anotherArray)
})
Loading

0 comments on commit 4d1397b

Please sign in to comment.