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

Revert "Revert "Debounce dispatched state diffs"" #3579

Merged
merged 9 commits into from
Aug 15, 2023
57 changes: 52 additions & 5 deletions background/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -508,27 +508,74 @@ export default class Main extends BaseService<never> {
// Start up the redux store and set it up for proxying.
this.store = initializeStore(this, savedReduxState)

/**
* Tracks pending updates to the redux store. This is used to delay responding
* to dispatched thunks until the store has been updated in the UI. This is
* necessary to prevent race conditions where the UI expects the store to be
* updated before the thunk has finished dispatching.
*/
let storeUpdateLock: Promise<void> | null
let releaseLock: () => void

const queueUpdate = debounce(
(lastState, newState, updateFn) => {
if (lastState !== newState) {
const diff = deepDiff(lastState, newState)

if (diff !== undefined) {
updateFn(newState, [diff])
}
}
releaseLock()
},
30,
{ maxWait: 30, trailing: true }
)

wrapStore(this.store, {
serializer: encodeJSON,
deserializer: decodeJSON,
diffStrategy: (oldObj, newObj) => {
const diffWrapper = deepDiff(oldObj, newObj)
return diffWrapper === undefined ? [] : [diffWrapper]
diffStrategy: (oldObj, newObj, forceUpdate) => {
if (!storeUpdateLock) {
storeUpdateLock = new Promise((resolve) => {
releaseLock = () => {
resolve()
storeUpdateLock = null
}
})
}
Comment on lines +539 to +546
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Golfing at this point, but:

Suggested change
if (!storeUpdateLock) {
storeUpdateLock = new Promise((resolve) => {
releaseLock = () => {
resolve()
storeUpdateLock = null
}
})
}
storeUpdateLock ??= new Promise((resolve) => {
releaseLock = () => {
resolve()
storeUpdateLock = null
}
}


queueUpdate(oldObj, newObj, forceUpdate)

// Return no diffs as we're manually handling these inside `queueUpdate`
return []
},
dispatchResponder: async (
dispatchResult: Promise<unknown>,
dispatchResult: Promise<unknown> | unknown,
send: (param: { error: string | null; value: unknown | null }) => void
) => {
try {
// if dispatch is a thunk, wait for the result
const result = await dispatchResult

// By this time, all pending updates should've been tracked.
// since we're dealing with a thunk, we need to wait for
// the store to be updated
await storeUpdateLock

send({
error: null,
value: encodeJSON(await dispatchResult),
value: encodeJSON(result),
})
} catch (error) {
logger.error(
"Error awaiting and dispatching redux store result: ",
error
)

// Store could still have been updated if there was an error
Shadowfiend marked this conversation as resolved.
Show resolved Hide resolved
await storeUpdateLock

send({
error: encodeJSON(error),
value: null,
Expand Down
65 changes: 65 additions & 0 deletions background/tests/differ.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import assert from "assert"
import { diff, patch } from "../differ"

describe("Diffing & Patching", () => {
describe("BigInts", () => {
const cases = [
0,
"test",
2n,
{ b: 1 },
[5],
null,
undefined,
true,
false,
"",
NaN,
]

test("Creates correct patches when diffing against bigints", () => {
cases.forEach((initial) => {
const expected = 9999n
const delta = diff(initial, expected)

assert(delta)
expect(patch(initial, delta)).toEqual(expected)
})
})

test("Creates correct patches when diffing from bigints", () => {
cases.forEach((expected) => {
const initial = 9999n
const delta = diff(initial, expected)

assert(delta)
expect(patch(initial, delta)).toEqual(expected)
})
})

test("Creates correct patches regardless of depth of change", () => {
cases.forEach((expected) => {
const targetFrom = { a: { b: [0, { c: 9999n }] } }
const target = { a: { b: [0, { c: expected }] } }

const delta = diff(targetFrom, target)

assert(delta)

expect(patch(targetFrom, delta)).toEqual(target)
})

// against bigints
cases.forEach((initial) => {
const targetFrom = { a: { b: [0, { c: initial }] } }
const target = { a: { b: [0, { c: 9999n }] } }

const delta = diff(targetFrom, target)

assert(delta)

expect(patch(targetFrom, delta)).toEqual(target)
})
})
})
})
1 change: 1 addition & 0 deletions e2e-tests/utils/onboarding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ export default class OnboardingHelper {
await page
.getByTestId("remaining_seed_words")
.getByRole("button", { name: word })
.first() // can be a duplicate word
.click()
}

Expand Down
Loading