Fix Acuant sequence-break error cookie sync issue

[aduth]

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. Thus, the next time the user clicks "Take Photo", the cookie value logic 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.

Steps to reproduce:

1. Visit document capture step on iOS 15
2. Attempt to "Take Photo" until a sequence break error occurs (dialog closes and "an error on our end" message is displayed)
3. Click "Take Photo"
4. Take a photo using the native camera UI

Before: The photo value is never assigned to the field
After: The photo value is assigned to the field

[zachmargolis]

LGTM

[zachmargolis]

WDYT of notifySubscribers or something along those lines? My first thought was that it was going to load a newer copy of the data

[aduth]

My first thought was that it was going to load a newer copy of the data

That is effectively what it's doing, though maybe not entirely clear. The setState (setSubscriberState) will pull the latest cookie value from getValue passed as a callback:

https://reactjs.org/docs/hooks-reference.html#functional-updates

Re-reading this documentation, it might be unnecessary to do the functional update, when we could just do setSubscriberValue(getValue()) or ...:

const nextValue = getValue();
const subscribers = /** @type {Subscribers} */ (subscriptions.get(name));
subscribers.forEach((setSubscriberValue) => setSubscriberValue(nextValue));

I'm not sure if notifySubscribers would be better since it might imply that the value has already been updated at that point, and all that's left is to let the subscribers know about it. That's true for the cookie value itself, but the individual states still need to be set.

[zachmargolis]

yeah it looks to me like the code was notifying the subscribers to pull a new value, rather than using a value in that instance of refreshAcuantFailureCookie(), but digging in, yes it's clear what it does so I wouldn't sweat that too much about naming here

[aduth]

Adjusted this a bit in e72e1aa to pull out value assignment. Might be a bit clearer that the new value is being fetched and assigned?

[zachmargolis]

That is definitely clearer, but not what I was focusing on when I kicked off this thread

I was looking at the usage instance here:
https://github.com/18F/identity-idp/pull/5830/files#diff-aae034b49f7f9dd513819dda9adfd0c973268797cf3cb935333dd2c73b8fb070R539
and when I read refreshAcuantFailureCookie() on that line, that's where I got confused about if it was updating value in-place or just notifying subscribers they should refresh, because it was just a void function call so the value couldn't be used directly there.

Then I traced back where that function was defined to the prototype here and commented on the example name.

[aduth]

that's where I got confused about if it was updating value in-place or just notifying subscribers they should refresh, because it was just a void function call so the value couldn't be used directly there.

Ah, okay. I think I understand a little better.

One idea that's been lingering in the back of my mind is that this wouldn't have been an issue if we had gone with the getter approach discussed in #5778. I still don't love the idea of a getter, at least purely from the perspective of not triggering a re-render on updates, but: Maybe it could be adapted into a compromise where (a) we use it as a getter and (b) we can "refresh"/"sync" the value by setting it to the latest value.

So instead of:

return [value, setValue, refreshValue];

...we have:

return [getValue, setValue];

...where refreshValue can be emulated by setValue(getValue()).

Would that do anything to improve the clarity of how values are being updated over time?

[zachmargolis]

The getter approach makes sense, and like you said, it's not The React Way, but cookies are weird and this is a pretty limited scope, I don't expect we'd use that pattern elsewhere if we didn't have to. But that said, the refreshValue also makes sense so I really don't know anymore.

[aduth]

Alright, in that case I might just keep it as-is just to keep things simple and avoid any [more] unintended regressions.