Skip to content

Commit

Permalink
Support out of bound indices on proxied arrays (#3559)
Browse files Browse the repository at this point in the history
  • Loading branch information
urugator committed Nov 1, 2022
1 parent cbd9811 commit 8cf4784
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 56 deletions.
5 changes: 5 additions & 0 deletions .changeset/grumpy-dots-wink.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"mobx": minor
---

Proxied observable arrays can now safely read/write out of bound indices. See https://github.com/mobxjs/mobx/discussions/3537
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ exports[`very long arrays can be safely passed to nativeArray.concat #2379 1`] =
[MockFunction] {
"calls": Array [
Array [
"[mobx] Out of bounds read: 10000",
"[mobx.array] Attempt to read an array index (10000) that is out of bounds (10000). Please check length first. Out of bound indices will not be tracked by MobX",
],
],
"results": Array [
Expand Down
17 changes: 0 additions & 17 deletions packages/mobx/__tests__/v5/base/__snapshots__/array.js.snap

This file was deleted.

40 changes: 16 additions & 24 deletions packages/mobx/__tests__/v5/base/array.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ const mobx = require("../../../src/mobx.ts")
const { observable, when, _getAdministration, reaction, computed, makeObservable, autorun } = mobx
const iterall = require("iterall")

let consoleWarnMock
let consoleWarnSpy
afterEach(() => {
consoleWarnMock?.mockRestore()
consoleWarnSpy?.mockRestore()
})

test("test1", function () {
Expand Down Expand Up @@ -402,26 +402,6 @@ test("array exposes correct keys", () => {
expect(keys).toEqual(["0", "1"])
})

test("accessing out of bound values throws", () => {
const a = mobx.observable([])

let warns = 0
const baseWarn = console.warn
console.warn = () => {
warns++
}

a[0] // out of bounds
a[1] // out of bounds

expect(warns).toBe(2)

expect(() => (a[0] = 3)).not.toThrow()
expect(() => (a[2] = 4)).toThrow(/Index out of bounds, 2 is larger than 1/)

console.warn = baseWarn
})

test("replace can handle large arrays", () => {
const a = mobx.observable([])
const b = []
Expand Down Expand Up @@ -670,9 +650,7 @@ test("very long arrays can be safely passed to nativeArray.concat #2379", () =>
expect(longObservableArray).toEqual(longNativeArray)
expect(longObservableArray[9000]).toBe(longNativeArray[9000])
expect(longObservableArray[9999]).toBe(longNativeArray[9999])
consoleWarnMock = jest.spyOn(console, "warn").mockImplementation(() => {})
expect(longObservableArray[10000]).toBe(longNativeArray[10000])
expect(consoleWarnMock).toMatchSnapshot()

const expectedArray = nativeArray.concat(longNativeArray)
const actualArray = nativeArray.concat(longObservableArray)
Expand Down Expand Up @@ -867,3 +845,17 @@ test("reduce without initial value #2432", () => {
expect(observableArraySum).toEqual(arraySum)
expect(arrayReducerArgs).toEqual(observableArrayReducerArgs)
})

test("accessing out of bound indices is supported", () => {
consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => {
throw new Error(`Unexpected console.warn call`)
})

const array = observable([])

array[1]
array[2]
array[1001] = "foo"
expect(array.length).toBe(1002)
expect(array[1001]).toBe("foo")
})
37 changes: 23 additions & 14 deletions packages/mobx/src/types/observablearray.ts
Original file line number Diff line number Diff line change
Expand Up @@ -345,19 +345,24 @@ export class ObservableArrayAdministration
}

get_(index: number): any | undefined {
if (index < this.values_.length) {
this.atom_.reportObserved()
return this.dehanceValue_(this.values_[index])
}
console.warn(
__DEV__
? `[mobx] Out of bounds read: ${index}`
: `[mobx.array] Attempt to read an array index (${index}) that is out of bounds (${this.values_.length}). Please check length first. Out of bound indices will not be tracked by MobX`
)
if (this.legacyMode_ && index >= this.values_.length) {
console.warn(
__DEV__
? `[mobx.array] Attempt to read an array index (${index}) that is out of bounds (${this.values_.length}). Please check length first. Out of bound indices will not be tracked by MobX`
: `[mobx] Out of bounds read: ${index}`
)
return undefined
}
this.atom_.reportObserved()
return this.dehanceValue_(this.values_[index])
}

set_(index: number, newValue: any) {
const values = this.values_
if (this.legacyMode_ && index > values.length) {
// out of bounds
die(17, index, values.length)
}
if (index < values.length) {
// update at index in range
checkIfStateModificationsAreAllowed(this.atom_)
Expand All @@ -380,12 +385,16 @@ export class ObservableArrayAdministration
values[index] = newValue
this.notifyArrayChildUpdate_(index, newValue, oldValue)
}
} else if (index === values.length) {
// add a new item
this.spliceWithArray_(index, 0, [newValue])
} else {
// out of bounds
die(17, index, values.length)
// For out of bound index, we don't create an actual sparse array,
// but rather fill the holes with undefined (same as setArrayLength_).
// This could be considered a bug.
const newItems = new Array(index + 1 - values.length)
for (let i = 0; i < newItems.length - 1; i++) {
newItems[i] = undefined
} // No Array.fill everywhere...
newItems[newItems.length - 1] = newValue
this.spliceWithArray_(values.length, 0, newItems)
}
}
}
Expand Down

0 comments on commit 8cf4784

Please sign in to comment.