From 37c1cf0a6ccbc573aa6d8a4e5324ec752f289061 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Mon, 5 Sep 2022 18:58:18 +0200 Subject: [PATCH 01/10] fix --- .changeset/fresh-suns-listen.md | 5 +++++ .../mobx/src/types/legacyobservablearray.ts | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 .changeset/fresh-suns-listen.md diff --git a/.changeset/fresh-suns-listen.md b/.changeset/fresh-suns-listen.md new file mode 100644 index 000000000..320dc290a --- /dev/null +++ b/.changeset/fresh-suns-listen.md @@ -0,0 +1,5 @@ +--- +"mobx": patch +--- + +fix regression #3514: LegacyObservableArray compat with Safari 9.\* diff --git a/packages/mobx/src/types/legacyobservablearray.ts b/packages/mobx/src/types/legacyobservablearray.ts index 253f0c177..dab40117d 100644 --- a/packages/mobx/src/types/legacyobservablearray.ts +++ b/packages/mobx/src/types/legacyobservablearray.ts @@ -14,6 +14,21 @@ import { defineProperty } from "../internal" +// Bug in safari 9.* (or iOS 9 safari mobile). See #364 +const ENTRY_0 = createArrayEntryDescriptor(0) + +const safariPrototypeSetterInheritanceBug = (() => { + let v = false + const p = {} + Object.defineProperty(p, "0", { + set: () => { + v = true + } + }) + Object.create(p)["0"] = 1 + return v === false +})() + /** * This array buffer contains two lists of properties, so that all arrays * can recycle their property definitions, which significantly improves performance of creating @@ -57,6 +72,12 @@ class LegacyObservableArray extends StubArray { this.spliceWithArray(0, 0, initialValues) allowStateChangesEnd(prev) } + + if (safariPrototypeSetterInheritanceBug) { + // Seems that Safari won't use numeric prototype setter untill any * numeric property is + // defined on the instance. After that it works fine, even if this property is deleted. + Object.defineProperty(this, "0", ENTRY_0) + } } concat(...arrays: T[][]): T[] { From 6cca25748b1a63f1cac10f0387d4221d83e3a93f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Mon, 31 Oct 2022 23:53:39 +0100 Subject: [PATCH 02/10] fix-3537 --- .changeset/grumpy-dots-wink.md | 5 ++ packages/mobx/__tests__/v5/base/array.js | 66 ++++++++++++++++------ packages/mobx/src/types/observablearray.ts | 37 +++++++----- 3 files changed, 77 insertions(+), 31 deletions(-) create mode 100644 .changeset/grumpy-dots-wink.md diff --git a/.changeset/grumpy-dots-wink.md b/.changeset/grumpy-dots-wink.md new file mode 100644 index 000000000..a9ae22d5d --- /dev/null +++ b/.changeset/grumpy-dots-wink.md @@ -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 diff --git a/packages/mobx/__tests__/v5/base/array.js b/packages/mobx/__tests__/v5/base/array.js index fa23d0c83..5ef2de096 100644 --- a/packages/mobx/__tests__/v5/base/array.js +++ b/packages/mobx/__tests__/v5/base/array.js @@ -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 () { @@ -402,8 +402,8 @@ test("array exposes correct keys", () => { expect(keys).toEqual(["0", "1"]) }) -test("accessing out of bound values throws", () => { - const a = mobx.observable([]) +test("legacy array: accessing out of bound values throws", () => { + const a = mobx.observable([], { proxy: false }) let warns = 0 const baseWarn = console.warn @@ -666,25 +666,41 @@ test("very long arrays can be safely passed to nativeArray.concat #2379", () => const nativeArray = ["a", "b"] const longNativeArray = [...Array(10000).keys()] // MAX_SPLICE_SIZE seems to be the threshold const longObservableArray = observable(longNativeArray) + const longLegacyArray = observable(longNativeArray, { proxy: false }) expect(longObservableArray.length).toBe(10000) + expect(longLegacyArray.length).toBe(10000) + 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() + expect(longLegacyArray).toEqual(longNativeArray) - const expectedArray = nativeArray.concat(longNativeArray) - const actualArray = nativeArray.concat(longObservableArray) + expect(longObservableArray[9000]).toBe(longNativeArray[9000]) + expect(longLegacyArray[9000]).toBe(longNativeArray[9000]) - expect(actualArray).toEqual(expectedArray) + expect(longObservableArray[9999]).toBe(longNativeArray[9999]) + expect(longLegacyArray[9999]).toBe(longNativeArray[9999]) - 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(longObservableArray[10000]).toBe(longNativeArray[10000]) + consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}) // Out of bound warn + expect(longLegacyArray[10000]).toBe(longNativeArray[10000]) + expect(consoleWarnSpy).toMatchSnapshot() + + const expectedNativeArray = nativeArray.concat(longNativeArray) + const actualObservableArray = nativeArray.concat(longObservableArray) + const actualLegacyArray = nativeArray.concat(longLegacyArray.slice()) // .slice because legacy isn't concat spreadable + + expect(actualObservableArray).toEqual(expectedNativeArray) + expect(actualLegacyArray).toEqual(expectedNativeArray) + + const anotherNativeArray = [0, 1, 2, 3, 4, 5] + const anotherObservableArray = observable(anotherNativeArray) + const anotherLegacyArray = observable(anotherNativeArray) + const r1 = anotherNativeArray.splice(2, 2, ...longNativeArray) + const r2 = anotherObservableArray.splice(2, 2, ...longNativeArray) + const r3 = anotherLegacyArray.splice(2, 2, ...longNativeArray) expect(r2).toEqual(r1) - expect(observableArray).toEqual(anotherArray) + expect(r3).toEqual(r1) + expect(anotherObservableArray).toEqual(anotherNativeArray) + expect(anotherLegacyArray).toEqual(anotherNativeArray) }) describe("dehances", () => { @@ -867,3 +883,19 @@ test("reduce without initial value #2432", () => { expect(observableArraySum).toEqual(arraySum) expect(arrayReducerArgs).toEqual(observableArrayReducerArgs) }) + +test("proxied arrays can access out-bound indices", () => { + 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") + + consoleWarnSpy.mockRestore() +}) diff --git a/packages/mobx/src/types/observablearray.ts b/packages/mobx/src/types/observablearray.ts index 6c720f2dd..4e2aa572a 100644 --- a/packages/mobx/src/types/observablearray.ts +++ b/packages/mobx/src/types/observablearray.ts @@ -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] 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` + ) + 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_) @@ -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) } } } From 9802ef4e5eaa303fb9cde354470081aa83d8c97f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Tue, 1 Nov 2022 00:04:22 +0100 Subject: [PATCH 03/10] dunno how it got in here --- .changeset/fresh-suns-listen.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changeset/fresh-suns-listen.md diff --git a/.changeset/fresh-suns-listen.md b/.changeset/fresh-suns-listen.md deleted file mode 100644 index 320dc290a..000000000 --- a/.changeset/fresh-suns-listen.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"mobx": patch ---- - -fix regression #3514: LegacyObservableArray compat with Safari 9.\* From 1bee07ba87547c8bc26ed5fa508ad0905202ba20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Tue, 1 Nov 2022 00:07:43 +0100 Subject: [PATCH 04/10] suprefluous mockRestore --- packages/mobx/__tests__/v5/base/array.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/mobx/__tests__/v5/base/array.js b/packages/mobx/__tests__/v5/base/array.js index 5ef2de096..518425abd 100644 --- a/packages/mobx/__tests__/v5/base/array.js +++ b/packages/mobx/__tests__/v5/base/array.js @@ -896,6 +896,4 @@ test("proxied arrays can access out-bound indices", () => { array[1001] = "foo" expect(array.length).toBe(1002) expect(array[1001]).toBe("foo") - - consoleWarnSpy.mockRestore() }) From 149580bb58d8c274cddceb4c7497410b343ad8f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Tue, 1 Nov 2022 13:01:26 +0100 Subject: [PATCH 05/10] fix swapped dev/prod warnings --- packages/mobx/src/types/observablearray.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/mobx/src/types/observablearray.ts b/packages/mobx/src/types/observablearray.ts index 4e2aa572a..d5e46a03a 100644 --- a/packages/mobx/src/types/observablearray.ts +++ b/packages/mobx/src/types/observablearray.ts @@ -348,8 +348,8 @@ export class ObservableArrayAdministration if (this.legacyMode_ && index >= this.values_.length) { 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` + ? `[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 } From efae44a7108b533ff1df1de58b5c52783f520a00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Tue, 1 Nov 2022 13:03:36 +0100 Subject: [PATCH 06/10] fix test description --- packages/mobx/__tests__/v5/base/array.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx/__tests__/v5/base/array.js b/packages/mobx/__tests__/v5/base/array.js index 518425abd..552582495 100644 --- a/packages/mobx/__tests__/v5/base/array.js +++ b/packages/mobx/__tests__/v5/base/array.js @@ -884,7 +884,7 @@ test("reduce without initial value #2432", () => { expect(arrayReducerArgs).toEqual(observableArrayReducerArgs) }) -test("proxied arrays can access out-bound indices", () => { +test("proxied arrays can access out of bound indices", () => { consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => { throw new Error(`Unexpected console.warn call`) }) From 1b1b9d78b754edd7a5532b2e635977431fb51a45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Tue, 1 Nov 2022 13:31:21 +0100 Subject: [PATCH 07/10] update snapshot --- packages/mobx/__tests__/v5/base/__snapshots__/array.js.snap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx/__tests__/v5/base/__snapshots__/array.js.snap b/packages/mobx/__tests__/v5/base/__snapshots__/array.js.snap index 8363de5cf..1a4890e74 100644 --- a/packages/mobx/__tests__/v5/base/__snapshots__/array.js.snap +++ b/packages/mobx/__tests__/v5/base/__snapshots__/array.js.snap @@ -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 [ From 88c41f27827026cc6a2328168c7ff0218b4d2af6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Tue, 1 Nov 2022 13:43:06 +0100 Subject: [PATCH 08/10] update v4 snapshot --- packages/mobx/__tests__/v4/base/__snapshots__/array.js.snap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx/__tests__/v4/base/__snapshots__/array.js.snap b/packages/mobx/__tests__/v4/base/__snapshots__/array.js.snap index 8363de5cf..1a4890e74 100644 --- a/packages/mobx/__tests__/v4/base/__snapshots__/array.js.snap +++ b/packages/mobx/__tests__/v4/base/__snapshots__/array.js.snap @@ -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 [ From 7c54b738297784a46b5d0f9d333e047ca30f3cb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Tue, 1 Nov 2022 13:51:35 +0100 Subject: [PATCH 09/10] refactor tests --- .../v5/base/__snapshots__/array.js.snap | 17 ----- packages/mobx/__tests__/v5/base/array.js | 64 ++++--------------- 2 files changed, 13 insertions(+), 68 deletions(-) delete mode 100644 packages/mobx/__tests__/v5/base/__snapshots__/array.js.snap diff --git a/packages/mobx/__tests__/v5/base/__snapshots__/array.js.snap b/packages/mobx/__tests__/v5/base/__snapshots__/array.js.snap deleted file mode 100644 index 1a4890e74..000000000 --- a/packages/mobx/__tests__/v5/base/__snapshots__/array.js.snap +++ /dev/null @@ -1,17 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`very long arrays can be safely passed to nativeArray.concat #2379 1`] = ` -[MockFunction] { - "calls": Array [ - Array [ - "[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 [ - Object { - "type": "return", - "value": undefined, - }, - ], -} -`; diff --git a/packages/mobx/__tests__/v5/base/array.js b/packages/mobx/__tests__/v5/base/array.js index 552582495..025c56b7e 100644 --- a/packages/mobx/__tests__/v5/base/array.js +++ b/packages/mobx/__tests__/v5/base/array.js @@ -402,26 +402,6 @@ test("array exposes correct keys", () => { expect(keys).toEqual(["0", "1"]) }) -test("legacy array: accessing out of bound values throws", () => { - const a = mobx.observable([], { proxy: false }) - - 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 = [] @@ -664,43 +644,25 @@ test("correct array should be passed to callbacks #2326", () => { test("very long arrays can be safely passed to nativeArray.concat #2379", () => { const nativeArray = ["a", "b"] - const longNativeArray = [...Array(10000).keys()] // MAX_SPLICE_SIZE seems to be the threshold + const longNativeArray = [...Array(10000).keys()] const longObservableArray = observable(longNativeArray) - const longLegacyArray = observable(longNativeArray, { proxy: false }) expect(longObservableArray.length).toBe(10000) - expect(longLegacyArray.length).toBe(10000) - expect(longObservableArray).toEqual(longNativeArray) - expect(longLegacyArray).toEqual(longNativeArray) - expect(longObservableArray[9000]).toBe(longNativeArray[9000]) - expect(longLegacyArray[9000]).toBe(longNativeArray[9000]) - expect(longObservableArray[9999]).toBe(longNativeArray[9999]) - expect(longLegacyArray[9999]).toBe(longNativeArray[9999]) - expect(longObservableArray[10000]).toBe(longNativeArray[10000]) - consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}) // Out of bound warn - expect(longLegacyArray[10000]).toBe(longNativeArray[10000]) - expect(consoleWarnSpy).toMatchSnapshot() - - const expectedNativeArray = nativeArray.concat(longNativeArray) - const actualObservableArray = nativeArray.concat(longObservableArray) - const actualLegacyArray = nativeArray.concat(longLegacyArray.slice()) // .slice because legacy isn't concat spreadable - - expect(actualObservableArray).toEqual(expectedNativeArray) - expect(actualLegacyArray).toEqual(expectedNativeArray) - - const anotherNativeArray = [0, 1, 2, 3, 4, 5] - const anotherObservableArray = observable(anotherNativeArray) - const anotherLegacyArray = observable(anotherNativeArray) - const r1 = anotherNativeArray.splice(2, 2, ...longNativeArray) - const r2 = anotherObservableArray.splice(2, 2, ...longNativeArray) - const r3 = anotherLegacyArray.splice(2, 2, ...longNativeArray) + + const expectedArray = nativeArray.concat(longNativeArray) + const actualArray = nativeArray.concat(longObservableArray) + + 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(r3).toEqual(r1) - expect(anotherObservableArray).toEqual(anotherNativeArray) - expect(anotherLegacyArray).toEqual(anotherNativeArray) + expect(observableArray).toEqual(anotherArray) }) describe("dehances", () => { @@ -884,7 +846,7 @@ test("reduce without initial value #2432", () => { expect(arrayReducerArgs).toEqual(observableArrayReducerArgs) }) -test("proxied arrays can access out of bound indices", () => { +test("accessing out of bound values does NOT throw", () => { consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => { throw new Error(`Unexpected console.warn call`) }) From 487f1cbaa637c1ffdc13c2aaba3ad4b2290e3bff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Tue, 1 Nov 2022 14:07:15 +0100 Subject: [PATCH 10/10] finishing touches --- packages/mobx/__tests__/v5/base/array.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/mobx/__tests__/v5/base/array.js b/packages/mobx/__tests__/v5/base/array.js index 025c56b7e..8b9cc576c 100644 --- a/packages/mobx/__tests__/v5/base/array.js +++ b/packages/mobx/__tests__/v5/base/array.js @@ -644,7 +644,7 @@ test("correct array should be passed to callbacks #2326", () => { test("very long arrays can be safely passed to nativeArray.concat #2379", () => { const nativeArray = ["a", "b"] - const longNativeArray = [...Array(10000).keys()] + const longNativeArray = [...Array(10000).keys()] // MAX_SPLICE_SIZE seems to be the threshold const longObservableArray = observable(longNativeArray) expect(longObservableArray.length).toBe(10000) expect(longObservableArray).toEqual(longNativeArray) @@ -846,7 +846,7 @@ test("reduce without initial value #2432", () => { expect(arrayReducerArgs).toEqual(observableArrayReducerArgs) }) -test("accessing out of bound values does NOT throw", () => { +test("accessing out of bound indices is supported", () => { consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => { throw new Error(`Unexpected console.warn call`) })