Shadowing isn’t so bad

I introduced a bug the other month to CGA’s “Talk Time” Zoom app. This tool tracks the amount of time each lesson participant speaks for, and this data is an extremely valuable addition to our pastoral care strategy. While a student not speaking much in class isn’t a red flag in and of itself, changes in this data is a useful signal for our pastoral care team.

The app is fairly simple; whenever Zoom reports a change in the meeting’s active speaker(s) we store the user IDs of those speakers in state and start accumulating talk time statistics for those speakers. If you were to implement this logic yourself, then the code might end up looking something like this:

// Maps from a user ID -> interval IDconst [speakerIntervalIds, setSpeakerIntervalIds] = useState<Record<string, number | undefined>>({}); useEffect(() => {  // TODO: Logic for handling new speakerIds...   // Handle dropped speakerIds  setSpeakerIntervalIds((prev) => {    const speakerIdsWithInterval = Object.keys(prev);    const removedSpeakerIds = speakerIdsWithInterval.filter((id) => !speakerIds.includes(id));     const newSpeakerIntervals = { ...prev };    for (const speakerId of removedSpeakerIds) {      if (!newSpeakerIntervals[speakerId]) continue;       // Clear interval and remove it from our map      clearInterval(newSpeakerIntervals[speakerId]);      delete newSpeakerIntervals[speakerId];    }    return newSpeakerIntervals;  });}, [speakerIds]);

The code above works, but the problem is that I wrote something slightly different:

// ...    const newSpeakerIntervals = { ...prev };    for (const speakerId of removedSpeakerIds) {      // speakerIntervalIds wasn't in our dependency array :/      if (!speakerIntervalIds[speakerId]) continue;       clearInterval(speakerIntervalIds[speakerId]);      delete newSpeakerIntervals[speakerId];    }    return newSpeakerIntervals;// ...

My use of speakerIntervalIds here resulted in our Talk Time app failing to clear the intervals correctly, meaning that users who spoke multiple times in a lesson ended up with hugely inflated talk time statistics. We caught this bug before it hit production so no real user data was affected, but why did this happen in the first place?

There are a few reasons:

1. Our Talk Time app is using Next.js, and for some reason the linting configuration isn’t working nicely with our frontend monorepo. Ordinarily the react-hooks/exhaustive-deps rule would warn me about accessing speakerIntervalIds without listing it as a hook dependency, but in this case my editor wasn’t giving me any warnings. Manual pnpm lint commands (like the ones ran by our build pipelines) still worked, but…
2. We don’t fail our builds for linter warnings—we only fail them for errors. The react-hooks/exhaustive-deps rule is a warning which meant that I was able to merge my PR without incident.
3. We didn’t have a unit test for Talk Time tracking; at this point the project was very young and experimental, so we were focused on shipping an MVP as soon as possible to get a better sense of whether it was worth continuing. Tests are good for improving maintainability and reliability of a codebase, and neither of these are primary concerns for a greenfield project with a high chance of being cut.
4. I called my setState parameter prev. Because I did this, it was possible in the first place for me to refer to speakerIntervalIds! Shadowing that variable would have made it impossible for me to write this bug.

Points 1-3 are important deficits in our tooling which need to be fixed (and have, to varying degrees in the time between then and now!) but the point of this post is to really focus in on point 4. If I had called my setState parameter speakerIntervalIds instead of prev, it would have been impossible for me to write this bug. Why didn’t I just do that in the first place?

What is shadowing?

Shadowing is when a variable has the same name as a variable declared in an outer scope. When shadowing occurs, it becomes impossible to refer to that outer variable.

Here’s an example of shadowing.

const myVariable = 'foo'; function doSomething() {  const myVariable = 'bar';  if (true) {    const myVariable = 'baz';    console.log(myVariable); // baz  }  console.log(myVariable); // bar}

Depending on where you look, shadowing can be controversial. Both the Zig and Elm programming languages make shadowing a compiler error, and Airbnb’s ESLint configuration—what I used when I first started “serious” web development—also makes shadowing an error out of the box. In C++, it is generally frowned upon.

The argument put forward by these communities is that shadowing makes code harder to read, and that increase in difficulty ultimately results in more bugs being introduced to the codebase.

On the other hand, the Rust programming language considers shadowing to be idiomatic. The following code is perfectly kosher, even though we shadow the original declaration of data:

use itertools::Itertools; struct Company {  id: u32,  name: String,} fn main() {  let data = [    Company { id: 1, name: "Apple" },    Company { id: 2, name: "Google" },    Company { id: 1, name: "Apple" }, // dupe    Company { id: 3, name: "Microsoft" },  ];   // shadowing!  let data = data.iter().unique_by(|it| &it.id).collect::<Vec>();   // data is now free of dupes!}

Shadowing is a surprisingly powerful feature in Rust because of how the language is designed. The language has a number of features built in to it that make shadowing the obvious and most ergonomic choice when writing Rust.

The details of why shadowing works as well as it does in Rust are a bit too complicated to fit into this post, but in future I will expand on this more as part of my series on cool language features.

For now it’s enough to say that when I first started programming professionally I was put off by variable shadowing due to the content I had consumed while learning. These days—after writing a fair bit of Rust—I’m a lot more open to the idea of shadowing variables.

Shadowing in React

If you look at the React docs, you’ll see that the very first introduction to the functional setState pattern avoids performing any shadowing:

function handleClick() {  setAge(a => a + 1);  setAge(a => a + 1);  setAge(a => a + 1);}

Back when I learned React hooks were still a number of years off. Back then class components were the way to go, and under that paradigm it wasn’t possible to shadow state anyway. State was read via this.state, after all.

But I do remember a lot of tutorials that showed code examples where class components updated state using a pattern along the lines of this.setState((prevState) => ...) ^{1 1}, and that’s where my instinct for calling my functional setState parameter either prev or prevX comes from.

Kent C. Dodds called his parameter currentState in this tutorial from just before hooks landed, and here’s Ben Awad doing the same thing with a currentCount parameter in 2019 post-hooks.

I highlight these two as they’re contemporaneous thought leaders in the frontend world that a significant proportion of JavaScript developers will be familiar with. There are more examples of this pattern in the wild, both in real code bases and in smaller blogs.

Avoiding shadowing when using setState updater functions is a de facto standard in the React world.

After reflecting on this some more, I think that this is the wrong way of using the functional setState API. You should be shadowing in this context, because there’s almost no use case where you actually would want to access the outer state value anyway.

I’m willing to bet that in the overwhelming majority of cases, accessing state instead of prevState (or actualState, currentState, or whatever name you prefer) indicates a bug in your code. Under this theory, shadowing state results in code that is more robust.

const [count, setCount] = useState(0); return (  <button    onClick={() => {      // Impossible to introduce my bug      setCount(count => count + 1);    }  >    Increment ({count})  </button>);

In cases where you do want to access the outermost state value, I would argue that shadowing is still the best option. By shadowing your outer state you are forced to draw attention to your unusual code, and this gives you an opportunity to be more explicit.

Closures are really hard to understand. Dan Abramov has a gigantic 49-minute blog post on his site, and a good chunk of it is spent explaining closures. Referring to count inside a useEffect doesn’t give you the most recent count value—it gives you the value of count at the time the effect’s closure was created. I’ve seen engineers all over the experience spectrum get tripped by this.

Here’s an example of how you can explicitly capture the outermost state value prior to shadowing:

useEffect(() => {  // IMO it feels clearer that `capturedState` refers to  // the value of `state` captured by this closure, instead of  // the most recent value. You can also add a comment here  // explaining your code to your team.  const capturedState = state;  setTimeout(() => setState((state) => {    // Do something with state + capturedState  }), 1_000);}, [state]);

As you can see, you don’t lose any capabilities when shadowing in this manner. It’s still possible to use the outermost value in the 1% of cases where you need to, but in the other 99% of cases you completely remove an entire category of bug. That’s a pretty good tradeoff!

Takeaways

I have three big learnings from this experience:

1. It’s really easy to be trapped by dogma, or fall into a pattern of doing things "just because."
2. The react-hooks/exhaustive-deps ESLint rule should be configured as an error instead of a warning.
3. Functional setState calls should (probably) always use shadowing to prevent this bug from being possible.

The first point here is particularly poignant for me. I like to think of myself as an independent thinker, but after writing this bug and reflecting on the root cause it’s clear that I’ve been dogmatically calling my setState parameters prevState for years without really thinking about why.

I hadn’t ran into an issue up until now, and so pattern matching based on my previous experience was sufficient. I think almost everyone in the software industry is vulnerable to this; and there’s a really good tweet by Dan Abramov that’s relevant:

At some point I realized that the famous developers who created tools I enjoyed and whose opinion I valued don’t always speak with deep consideration. That they’re normal people whose expertise is limited to what they’ve seen, and who rely on pattern-matching from experience.

— Dan Abramov

In light of this, I will (almost) always be shadowing my variables when using setState updater functions going forward. I think that this is a superior way of writing code, and it’s also a good example of how branching out and learning other pieces of technology can benefit your main line of work. If I hadn’t written as much Rust as I have over the past few years, then I don’t think I would have been able to connect this bug to a lack of shadowing.

There are some really cool ideas floating around in the software ecosystem—like guard statements in Swift—and it’s worth playing with these things to challenge your preconceptions and expand your thinking.

The other closely related takeaway I have from this is to bump up the react-hooks/exhaustive-deps linter rule from a warning to an error in all of my projects. There are some legitimate use cases where ignoring this rule makes sense, but in most cases a missing dependency will absolutely cause an error down the line ².

Both eslint-config-react-app and eslint-config-next come with this rule configured as a warning out of the box. Had this rule had been configured as an error, we would have never even merged this bug because our PR tests would have caught it.

Swapping it over to an error is a one line change to your ESLint config:

module.exports = {  // ...  rules: {    'react-hooks/exhaustive-deps': 'error',    // ...  },  // ...};