Change useCookie hook to getter, avoid global override#5778
Conversation
**Why**: As observed in #5747, the current logic can be prone to race conditions when multiple instances of the hook exist (mostly affecting specs). The implementation overriding the global was meant to support a use-case where a component would re-render in response to an external script modifying a cookie (e.g. Acuant SDK). Since our current sole usage is to get the cookie value at the time of a user initiating a capture, this live-updating support isn't necessary and unnecessarily complicates the implementation.
| function setValue(nextValue) { | ||
| const cookieValue = nextValue === null ? '; Max-Age=0' : nextValue; | ||
| document.cookie = `${name}=${cookieValue}`; | ||
| setStateValue(nextValue); |
There was a problem hiding this comment.
One might argue that we could just keep this in state, since internally calling setValue would trigger a re-render. The problem with this is that the value could fall out of sync with multiple instances of the hook (aside: probably want a test case for this). The getter isn't prone to this, since it'll get the value at the time that it's called.
Alternatively, we could introduce some simple shared context which makes sure that if the cookie value is updated, all instances of the hook are re-rendered with the new value.
There was a problem hiding this comment.
This seems pretty straightforward, I think it's clearer than using a shared context
There was a problem hiding this comment.
The problem with this is that the value could fall out of sync with multiple instances of the hook (aside: probably want a test case for this).
Added in cd4d91f.
This seems pretty straightforward, I think it's clearer than using a shared context
I was curious, so I tried in 1f493d5 to see what this might look like. It's a bit more complex, yeah, though not too bad I think? I guess I'm mostly concerned that the hook may give a false sense that things are kept well in-sync.
Specifically, this assertion would fail without these changes:
There was a problem hiding this comment.
I guess I'm mostly concerned that the hook may give a false sense that things are kept well in-sync.
Yeah that's my main reason for thinking the getter function is better, because it is the closest thing to a "live" value
There was a problem hiding this comment.
Yeah that's my main reason for thinking the getter function is better, because it is the closest thing to a "live" value
Problem is that it's live at the time that it's called, but in a case where we use the value as part of the render logic of a component, only the component instance which calls the setter would trigger an re-render, and any other component instance would continue showing a stale value.
There was a problem hiding this comment.
oh right I forget how "lazy" react is 😩 happy to trust your judgement on what is worthwhile vs overkill for something like this
There was a problem hiding this comment.
I wish it were easier to manage a "global" value than juggling subscribers like this, but I think it's fine enough. I'd also like to limit our use of "force render" vs. built-in alternatives like setState, so I feel a bit happier with this approach.
| let originalConsoleError; | ||
| beforeEach(() => { | ||
| console.unverifiedCalls = []; | ||
| sinon.stub(console, 'error').callsFake((message, ...args) => { | ||
| originalConsoleError = console.error; | ||
| console.error = sinon.stub().callsFake((message, ...args) => { | ||
| console.unverifiedCalls = console.unverifiedCalls.concat(format(message, ...args)); | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| console.error.restore(); | ||
| console.error = originalConsoleError; |
There was a problem hiding this comment.
I didn't mean to include the changes here, but they are necessary for an upgrade to @testing-libary/react-hooks, which I had temporarily updated in a misguided effort to resolve a failing spec. I guess I'll just leave it, since we'll need it for that upgrade anyways.
**Why**: To fix a bug where if the Acuant camera fails to start due to iOS 15 "sequence-break" error, subsequent attempts to capture would result in the captured image not being set correctly. This fixes a regression introduced in #5778 where the cookie value tracked by AcuantCapture falls out of date after Acuant handles the error, since we never refresh the value after it is [set internally by Acuant's error handling](https://github.com/Acuant/JavascriptWebSDKV11/blob/9a5387576a33710188501ad6233f986b1b6bb1cb/SimpleHTMLApp/webSdk/dist/AcuantCamera.js#L616). Thus, the next time the user clicks "Take Photo", the [cookie value logic](https://github.com/18F/identity-idp/blob/eceea0f4df59cf3bb934b561e1d141edf1276174/app/javascript/packages/document-capture/components/acuant-capture.jsx#L400) is out-of-date, and Acuant capture is started instead of the default manual capture handling. To resolve this, we anticipate that Acuant would set the cookie in their error handling of sequence-break, and refresh the cookie value when we get a chance to handle the error in our own code.
* Fix Acuant sequence-break error cookie sync issue **Why**: To fix a bug where if the Acuant camera fails to start due to iOS 15 "sequence-break" error, subsequent attempts to capture would result in the captured image not being set correctly. This fixes a regression introduced in #5778 where the cookie value tracked by AcuantCapture falls out of date after Acuant handles the error, since we never refresh the value after it is [set internally by Acuant's error handling](https://github.com/Acuant/JavascriptWebSDKV11/blob/9a5387576a33710188501ad6233f986b1b6bb1cb/SimpleHTMLApp/webSdk/dist/AcuantCamera.js#L616). Thus, the next time the user clicks "Take Photo", the [cookie value logic](https://github.com/18F/identity-idp/blob/eceea0f4df59cf3bb934b561e1d141edf1276174/app/javascript/packages/document-capture/components/acuant-capture.jsx#L400) is out-of-date, and Acuant capture is started instead of the default manual capture handling. To resolve this, we anticipate that Acuant would set the cookie in their error handling of sequence-break, and refresh the cookie value when we get a chance to handle the error in our own code. * Try to improve async handling of focus return * useCookie: Set next value as variable for subscriber set 1. Clearer that the value is actually being updated 2. Only parse cookie once for all subscribers, rather than per-subscriber
* Fix Acuant sequence-break error cookie sync issue **Why**: To fix a bug where if the Acuant camera fails to start due to iOS 15 "sequence-break" error, subsequent attempts to capture would result in the captured image not being set correctly. This fixes a regression introduced in #5778 where the cookie value tracked by AcuantCapture falls out of date after Acuant handles the error, since we never refresh the value after it is [set internally by Acuant's error handling](https://github.com/Acuant/JavascriptWebSDKV11/blob/9a5387576a33710188501ad6233f986b1b6bb1cb/SimpleHTMLApp/webSdk/dist/AcuantCamera.js#L616). Thus, the next time the user clicks "Take Photo", the [cookie value logic](https://github.com/18F/identity-idp/blob/eceea0f4df59cf3bb934b561e1d141edf1276174/app/javascript/packages/document-capture/components/acuant-capture.jsx#L400) is out-of-date, and Acuant capture is started instead of the default manual capture handling. To resolve this, we anticipate that Acuant would set the cookie in their error handling of sequence-break, and refresh the cookie value when we get a chance to handle the error in our own code. * Try to improve async handling of focus return * useCookie: Set next value as variable for subscriber set 1. Clearer that the value is actually being updated 2. Only parse cookie once for all subscribers, rather than per-subscriber (cherry picked from commit e8dcc56)
3 participants
Extracted from: #5747
Why: As observed in #5747, the current logic can be prone to race conditions when multiple instances of the hook exist (mostly affecting specs). The implementation overriding the global was meant to support a use-case where a component would re-render in response to an external script modifying a cookie (e.g. Acuant SDK). Since our current sole usage is to get the cookie value at the time of a user initiating a capture, this live-updating support isn't necessary and unnecessarily complicates the implementation.