DEV Community

Daniel Pertu
Daniel Pertu

Posted on

Deduplicating a constant did not stop it drifting

Most of our app respects the user's theme preference. A handful of marketing and auth pages do not: they are designed dark, they only look right dark, and they render dark whatever your OS says.

Implementing that is easy. Keeping it correct for two years, while other people add pages, is the actual problem. Ours broke twice, in the same way, for the same reason.

Why the list existed twice

To avoid a flash of the wrong theme you need the decision made before first paint, which means a blocking inline script in the document head:

<script
  dangerouslySetInnerHTML={{
    __html: `
      var forceDark = ['/', '/login', '/pricing', /* ... */]
      if (forceDark.includes(location.pathname)) {
        document.documentElement.classList.add('dark')
      }
    `,
  }}
/>
Enter fullscreen mode Exit fullscreen mode

That handles the first load. But this is a single page app, so a client-side navigation from a themed page to a forced-dark one never re-runs that script. So there was also a component:

// components/shared/ForceDarkMode.tsx
const FORCE_DARK = ['/', '/login', '/pricing', /* ...again... */]
Enter fullscreen mode Exit fullscreen mode

Two lists. Different files. Different languages, effectively, since one is a string of JavaScript inside JSX and the other is real TypeScript. They had to agree exactly or the page would flash the wrong theme, and of course they did not: neither included the Aon or Korn Ferry pages, so those two rendered light beside nine identical dark siblings.

Nobody noticed for a while, because you only see it if you happen to visit those two specific pages with a light OS theme.

First fix: one list

Obvious, and correct as far as it goes. Pull the array into its own module and have both callers import it.

export const FORCE_DARK_PATHS: readonly string[] = [
  '/', '/login', '/signup', '/pricing', '/help', '/about',
  /* ... */
  '/games/shl', '/games/aon', '/games/korn-ferry',
]

/** True when the given pathname should be forced into dark mode. */
export function shouldForceDark(pathname: string): boolean {
  return FORCE_DARK_PATHS.includes(pathname)
}
Enter fullscreen mode Exit fullscreen mode

The inline script serialises the imported array rather than restating it. One definition, two consumers, no drift.

And then it drifted again.

The second drift, which one list did not prevent

We were adding test providers, each one getting a hub page at /games/<provider> plus a pile of game routes, scoring, and content. Two of these were built by different people working in parallel.

Both of them added a provider. Neither added a line to FORCE_DARK_PATHS, because there is no reason to know that file exists when the task is "add a provider". The list is not imported by anything you touch. Nothing fails. Nothing types wrong. The page just renders light.

Deduplication solved the "two copies disagree" problem. It did nothing at all about "one copy is incomplete", which is a different failure with a different fix.

Second fix: a test that knows the relationship

The real invariant is not "this list has no duplicates". It is "every provider has a hub page, and every provider hub page is forced dark". That relationship exists in somebody's head. So write it down somewhere that fails:

// This list has now drifted twice. The first time, Aon and Korn Ferry rendered
// light beside nine dark siblings; the second time it was Thomas and TestGroup,
// each added by a different provider worker that had no reason to know the file
// existed. Nothing derives the list, so only a test catches the omission.
describe('FORCE_DARK_PATHS', () => {
  it('covers every provider hub page', () => {
    const missing = ALL_PROVIDERS.filter(
      (provider) => !FORCE_DARK_PATHS.includes(`/games/${provider}`)
    )
    expect(missing).toEqual([])
  })

  it('matches exactly, so a game route under a hub is not forced dark', () => {
    expect(shouldForceDark('/games/shl')).toBe(true)
    expect(shouldForceDark('/games/shl/shl-verbal')).toBe(false)
  })

  it('has no duplicate entries', () => {
    expect(FORCE_DARK_PATHS.length).toBe(new Set(FORCE_DARK_PATHS).size)
  })
})
Enter fullscreen mode Exit fullscreen mode

Adding a provider without an entry is now a red test with the provider name in the failure message, instead of a page somebody has to happen to look at with the right OS setting.

The comment matters as much as the assertions. It says the list has drifted twice and names both incidents, so the next person who finds this test tedious knows it is not hypothetical.

Why not derive it and skip the list entirely?

Fair question, and for the provider pages you could: ALL_PROVIDERS.map(p => '/games/' + p).

We did not, for two reasons.

The list also contains twenty-odd paths that are not derivable from anything: /, /login, /pricing, /privacy, /cheating. Deriving half of it and hardcoding the other half means the file has two mechanisms and a reader has to work out which one governs a given path.

More importantly, the inline script has to serialise this into the document head. A flat array of strings is trivially JSON-safe. A computed value is one refactor away from dragging something non-serialisable into a <script> tag, and the failure mode of a broken inline theme script is a page-wide flash on every first load.

So: a flat, boring, explicitly listed array, with a test enforcing the derivable subset. The test gives you the safety of derivation without giving up the simplicity of a literal.

Exact match, on purpose

expect(shouldForceDark('/games/shl')).toBe(true)
expect(shouldForceDark('/games/shl/shl-verbal')).toBe(false)
Enter fullscreen mode Exit fullscreen mode

includes on the exact pathname, not startsWith. The hub page is marketing and is dark by design. The pages underneath it are the app, and the app respects your preference. Prefix matching would silently force-dark every future route under a hub, which is precisely the kind of thing nobody notices until a user asks why one screen ignores their light theme.

Whenever you write path matching, decide between exact and prefix deliberately, then write the test that pins the choice. Six months later the code cannot tell you which one was intended, and the test can.

Have a look

Set your OS to light mode, then:

The interesting test is that last one, because a single-list-but-inline-only implementation passes the first two checks and fails the third.

The takeaway

Deduplicating a constant fixes disagreement between copies. It does not fix incompleteness of the one copy that is left, and incompleteness is the failure you get once more than one person is adding features.

When a list has to stay in step with something else in the codebase, the list alone is not the fix. Either derive it, or write the test that asserts the relationship, and put the history of how it broke in a comment above it.

Top comments (1)

Collapse
 
raknaos profile image
Raknaos

This is the clearest write-up of the dedup-versus-completeness distinction I have read. Removing the second copy fixed the failure where two lists disagree; it does nothing for the failure where the single remaining list is incomplete, because incompleteness has no local symptom — nothing throws, nothing type-errors, the page just renders in the wrong theme for two paths nobody opens with a light OS setting.

Encoding the relationship as a test is the correct shape, and the comment naming both incidents is doing as much work as the assertions. A test that reads as arbitrary gets deleted by the next person who finds it tedious; one that says "this drifted twice and here is what broke" survives. I would add only that ALL_PROVIDERS itself is worth asserting against the router or the content collection, so a provider added without a hub page fails there rather than silently missing from both sides of the comparison.

On not deriving it: keeping the flat array for the inline script is a real constraint, not laziness. A serialised computed value in the document head is the kind of thing that works until a refactor makes it a function, and the flash-of-wrong-theme bug it reintroduces is exactly the one the list exists to prevent.