Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 31 additions & 23 deletions app/javascript/packages/document-capture/hooks/use-cookie.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
import { useState, useEffect } from 'react';
import { createContext, useContext, useState, useEffect } from 'react';

/** @typedef {import('react').Dispatch<A>} Dispatch @template A */
/** @typedef {import('react').SetStateAction<S>} SetStateAction @template S */

/**
* @typedef {Dispatch<SetStateAction<string|null>>[]} Subscribers
*/

const CookieSubscriberContext = createContext(/** @type {Map<string, Subscribers>} */ (new Map()));

/**
* React hook to access and manage a cookie value by name.
Expand All @@ -8,42 +17,41 @@ import { useState, useEffect } from 'react';
* @return {[value: string|null, setValue: (nextValue: string?) => void]}
*/
function useCookie(name) {
const getCookieValue = () =>
const getValue = () =>
document.cookie
.split(';')
.map((part) => part.trim().split('='))
.find(([key]) => key === name)?.[1] ?? null;

const [value, setStateValue] = useState(getCookieValue);
const subscriptions = useContext(CookieSubscriberContext);
const [value, setStateValue] = useState(getValue);

useEffect(() => {
if (!subscriptions.has(name)) {
subscriptions.set(name, []);
}

const subscribers = /** @type {Subscribers} */ (subscriptions.get(name));
subscribers.push(setStateValue);

return () => {
subscribers.splice(subscribers.indexOf(setStateValue), 1);
if (!subscribers.length) {
subscriptions.delete(name);
}
};
}, [name]);

/**
* @param {string?} nextValue Value to set, or null to delete the value.
*/
function setValue(nextValue) {
const cookieValue = nextValue === null ? '; Max-Age=0' : nextValue;
document.cookie = `${name}=${cookieValue}`;
setStateValue(nextValue);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems pretty straightforward, I think it's clearer than using a shared context

Copy link
Copy Markdown
Contributor Author

@aduth aduth Jan 5, 2022

Choose a reason for hiding this comment

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

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:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh right I forget how "lazy" react is 😩 happy to trust your judgement on what is worthwhile vs overkill for something like this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

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

useEffect(() => {
const originalCookieDescriptor = Object.getOwnPropertyDescriptor(Document.prototype, 'cookie');
Object.defineProperty(Document.prototype, 'cookie', {
...originalCookieDescriptor,
set(nextValue) {
originalCookieDescriptor?.set?.call(this, nextValue);
setStateValue(getCookieValue);
},
});

return () => {
Object.defineProperty(
Document.prototype,
'cookie',
/** @type {PropertyDescriptor} */ (originalCookieDescriptor),
);
};
}, []);

return [value, setValue];
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import sinon from 'sinon';
import { renderHook } from '@testing-library/react-hooks';
import useCookie from '@18f/identity-document-capture/hooks/use-cookie';

Expand All @@ -14,20 +15,15 @@ describe('document-capture/hooks/use-cookie', () => {
expect(value).to.equal('baz');
});

it('does not interfere with default cookie setting behavior', () => {
renderHook(() => useCookie('foo'));

document.cookie = 'foo=bar';

expect(document.cookie).to.equal('foo=bar');
});

it('sets a new cookie value', () => {
const { result } = renderHook(() => useCookie('foo'));
const render = sinon.stub().callsFake(() => useCookie('foo'));
const { result } = renderHook(render);

const [, setValue] = result.current;

render.resetHistory();
setValue('bar');
expect(render).to.have.been.calledOnce();

const [value] = result.current;

Expand All @@ -38,15 +34,39 @@ describe('document-capture/hooks/use-cookie', () => {
it('unsets a cookie value by null', () => {
document.cookie = 'foo=bar';

const { result } = renderHook(() => useCookie('foo'));
const render = sinon.stub().callsFake(() => useCookie('foo'));
const { result } = renderHook(render);

const [, setValue] = result.current;

render.resetHistory();
setValue(null);
expect(render).to.have.been.calledOnce();

const [value] = result.current;

expect(document.cookie).to.equal('');
expect(value).to.be.null();
});

it('returns the same updated value between instances', () => {
const render1 = sinon.stub().callsFake(() => useCookie('foo'));
const render2 = sinon.stub().callsFake(() => useCookie('foo'));
const { result: result1 } = renderHook(render1);
const { result: result2 } = renderHook(render2);

const [, setValue] = result1.current;

render1.resetHistory();
render2.resetHistory();
setValue('bar');
expect(render1).to.have.been.calledOnce();
expect(render2).to.have.been.calledOnce();

const [value1] = result1.current;
const [value2] = result2.current;

expect(value1).to.equal('bar');
expect(value2).to.equal('bar');
});
});
6 changes: 4 additions & 2 deletions spec/javascripts/support/console.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,17 @@ export function chaiConsoleSpy(chai, utils) {
* `chaiConsoleSpy` Chai plugin.
*/
export function useConsoleLogSpy() {
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;
Comment on lines +47 to +57
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

expect(console.unverifiedCalls).to.be.empty(
`Unexpected console logging: ${console.unverifiedCalls.join(', ')}`,
);
Expand Down