DEV Community: Dominic Elm

What makes AI-generated code slop?

Dominic Elm — Tue, 13 Oct 2026 13:00:13 +0000

Suppose we ask an agent to write a saveAll() function to report success only after every item has been saved. It gives us this:

async function saveAll(items) {
  items.forEach(async (item) => {
    await database.save(item);
  });

  return { ok: true };
}

There's an async function, and an await next to the database call. At a glance, the code has the expected shape, right? But forEach() doesn't wait for promises. This function can report success while writes are still pending. A rejected save won't reject the promise returned by saveAll(), either.

A test that checks for { ok: true } and confirms that database.save() was called for each item can still pass. We now have code and tests that look finished without checking the thing the requirement asked for.

That's the type of problem I want to talk about. We can generate code that looks finished before anyone has properly checked it, and someone else ends up doing the checking and fixing. Is this code that a modern frontier or open-weight model would generate? Very likely not but the point stands. It's code that looks convincingly ready and plausible, yet it's not. AI makes it much easier to produce that work faster than other people can verify it.

What I mean by slop

Everyone is talking about AI slop, and I'm increasingly unsure whether we're talking about the same thing. Some people mean broken code, others mean unnecessary complexity, or a pull request whose author can't explain what they've submitted. The label alone tells us very little about what needs fixing.

I went back and forth on a definition for a while. I didn't invent the term "slop", but I wanted to bring together the recurring problems I've seen:

LLM code slop is the production of AI-generated software artifacts whose apparent plausibility or production speed is not matched by verified semantic, architectural, security, or maintainability quality. This shifts a disproportionate share of validation, debugging, or maintenance costs to other people or systems.

Ironically, AI helped me turn my own rough blurb into that formal definition.

By "production", I mean the way we produce and hand over work. Finding a bug in an AI-assisted change doesn't, by itself, make the contribution slop. Bugs slip through good engineering practices too, and human-written code can be slop. The problem starts when we call work finished without checking it well enough for its intended use.

Software artifacts includes code, but also tests and design documents. A misleading vulnerability report can consume engineering time without adding a single line to a codebase. Generated tests can make an incorrect implementation look convincing.

"Apparent plausibility or production speed" describes how finished the work looks and how quickly we can generate it. Familiar code patterns and a passing test suite make a change easy to accept. Neither tells us how thoroughly it was checked.

Semantic quality asks whether the code does what the requirement says. For saveAll(), starting every write is different from completing every write successfully.

Architectural quality asks how well the change fits the surrounding system. A new helper can work correctly on its own and still duplicate a policy the repository already has.

Security asks whether the system prevents behavior it must not allow, including when someone deliberately tries to misuse it. Checking that the intended user can perform an action leaves open whether an unauthorized user can do it too.

Maintainability asks whether another engineer can understand and safely change what we've added. An implementation can produce the right result today while making future changes harder to reason about.

"Disproportionate" leaves room for collaboration. Reviewers should ask questions, and new contributors should get help. I'd call it slop when an author skips verification they should have done and leaves that work to the reviewer or whoever has to maintain the system.

How much verification is enough depends on what happens when the software goes wrong. A rough prototype with explicit limits can be appropriate for trying out an idea. A payment system that charges someone twice can leave them short on rent money. A clinical system that mixes up patient records can lead to treatment decisions based on the wrong information. Those systems need much more thorough checks, including what happens when something fails. AI-assisted code can meet those standards, but carrying prototype shortcuts into software people rely on creates work and risks for others.

Someone has to do the checking

Daniel Stenberg's experience with curl makes that cost pretty visible. In his post, Death by a thousand slops, he described the burden of low-quality vulnerability reports. Investigating a report involved three or four members of the security team, with each person spending perhaps half an hour and sometimes up to three hours. Some team members had only a few hours a week available for curl.

Those reports cost attention even when the claimed vulnerability didn't exist. Someone had to investigate and establish why the claim was wrong. Stenberg also described poor human submissions with the same effect. Generating a convincing report is cheap compared with the work needed to verify it.

Tests need checking too

For many changes, we rely on unit, integration, and end-to-end tests. Agents can generate plenty more alongside the implementation. But the tests themselves don't always get the same scrutiny as the code they're supposed to verify. A handful of people I talked to told me they skip spec files completely when reviewing a pull request. Once everything passes, it's easy to assume the change has been properly checked.

Just recently I came across a blog post that really resonated with me. In The Problem With Your AI Tests, Michael Bromley gives an example of a discount test that checks whether the resulting total is non-negative. That assertion accepts all sorts of incorrect totals. The test looks relevant, runs successfully, and still doesn't establish that the discount was calculated correctly.

The saveAll() tests have the same problem. Checking the return value and counting calls tells us nothing about when the writes finish. A capable model might spot the forEach() mistake immediately, but the same timing bug can be harder to find across several functions.

If running all writes at once is appropriate for this workload, we can use Promise.all():

async function saveAll(items) {
  await Promise.all(items.map((item) => database.save(item)));
  return { ok: true };
}

Assuming database.save() fulfills only when a write succeeds, this reports success only after every write succeeds. If a write rejects, saveAll() rejects too. Promise.all() rejects on the first failure; it doesn't cancel the other writes or roll back writes that already succeeded.

Promise.allSettled() waits for every write to settle, but a rejected write becomes an entry in the results array. It doesn't make the combined promise reject. Swapping it into the code above and still returning { ok: true } would report success even after failed writes. We'd need to inspect the results and throw or return a failure when any write failed.

The code also starts every write at once, which may exceed the database's limits. The requirement needs to establish the supported workload and how to handle partial failure. Correct promise handling alone doesn't settle those decisions.

A useful test would keep at least one write pending and check that saveAll() hasn't reported success, then complete the writes and check the result. Another would reject a write and check that the failure reaches the caller. These tests need to fail against the original version. Otherwise, we still haven't established that they catch the bug.

Once I'm happy with a plan, I often ask a separate agent to write the tests before I let another agent work on the implementation. That requires a clearly defined interface and expected behavior. In my experience, having no implementation to look at helps the agent write better black-box tests, focused on what the code should do without depending on how it works internally.

But black-box tests can be wrong too. The test-writing agent can misunderstand the requirement, assume the wrong expected result, or miss a failure case. Two agents working from the same ambiguous plan can agree on the same mistake. I still need to review the expected results against the requirements. For a discount calculation, that means checking the expected total against the pricing rules. For saveAll(), it means agreeing on what success and failure mean before either agent starts.

Once those acceptance tests are reviewed, I'd lock them for the implementation step. Keep the approved copy outside the implementation agent's write access and have an independent check run that copy against its changes. Telling the agent "don't edit the tests" helps, but it doesn't enforce that boundary.

The agent can propose a correction when a test is wrong, and it can add white-box tests for internal behavior. Those additions need review too. Changes to the approved expectations need a separate review against the requirements, including changes to fixtures or test configuration that could weaken what gets checked.

Reward hacking is one reason to protect that boundary. A model can learn to exploit an evaluation's loopholes to get rewarded without solving the task. OpenAI documented an agent changing a verification function to always return true, and Anthropic demonstrated learned cheating in a controlled training setup with known loopholes. Those experiments don't establish how often this happens in everyday use. OpenAI also reported improper test edits in internal deployments in March 2026, classifying them as uncommon. A weak test may simply reflect a misunderstanding. In either case, changes that make a test easier to pass deserve scrutiny.

I'm also using mutation testing to check whether tests catch the failures they're supposed to. The idea is to introduce a small, deliberate fault into working code and rerun the tests. If a relevant test fails, it caught the mutation. If the tests still pass, we need to investigate what they missed. For saveAll(), useful faults to try include returning before writes finish or swallowing a write failure. For a retry policy, we could allow one more attempt than the agreed limit. These can be targeted mutations applied by hand when the tool doesn't generate them.

I've put that workflow into my mutation-test-review skill. For now, it supports Rust through cargo-mutants and TypeScript/JavaScript through StrykerJS. It reviews the assertions, runs mutations against the relevant code, and investigates survivors. It also checks whether the mutation run actually executed the expected tests and verifies suggested tests against the faults they're meant to catch.

A mutation score still needs interpretation. Some mutations leave the behavior unchanged, so no test could detect them. Others fail to compile or affect code whose tests weren't included in the run. A high score only tells us about the mutations and tests we ran. It can't establish that our requirements are right or cover a behavior nobody thought to specify. I want to know which realistic faults get through and what test would catch them.

Code that passes and still creates work

Some failures are harder to expose with a failing test, especially in complex systems. Suppose a repository already has a retry policy, say exponential backoff, but an agent adds another implementation elsewhere in the codebase. The new code might handle every example in the prompt correctly, but the team and our agents now have to maintain the same behavior in two places. This means a future change to retry rules now requires someone to find both implementations and work out whether their differences are intentional. The agent may simply have failed to discover the existing retry utilities.

This is one part of getting a repo "agent ready", which is to make it clear how shared problems are supposed to be solved. If retries go through an existing policy, the code and documentation should make that easy to find and reuse. In practice, this means clear repository structure and documented conventions which help agents plan and check their work.

I find OpenAI's write-up on harness engineering really interesting, and it describes why this matters in practice. The team found that agents copied existing patterns, including the ones they didn't want spreading through the codebase. They introduced rules that favor shared utility packages over separate helpers and enforced architectural boundaries with custom linters and structural tests. That gives agents feedback when a change breaks a repository rule, without waiting for someone to point it out in review.

For the retry example, I'd want the agent to find the existing policy before writing another one and explain why retries need to be handled differently. Reusing the same patterns for the same problems makes that decision easier for both people and agents. Where we can enforce a rule automatically, we should. We still need reviews for decisions the tooling can't make or for which it's missing important context, including whether two similar implementations really need to stay separate.

I have also seen a lot of unnecessary defensive code which creates similar work. If an internal contract guarantees a string, then adding layers of type checks and a broad catch that returns an empty string needs a very specific reason. In my opinion, agents operate on a lot of assumptions that don't necessarily hold up in the real world, and that makes the code much harder to understand and maintain.

But even worse, this kind of code may hide a broken assumption that the system should actually expose. Reviewers now have to understand fallback behavior that the requirement never asked for.

I've also noticed PRs getting rubber-stamped without a proper review. If a manual review amounts to clicking approve, we should question whether it adds value. Production staying online gives us some evidence, but a system can stay available while producing incorrect results or becoming harder to maintain.

Humans still have to take responsibility for production systems, and we need confidence in what we ship. That can come from reviewing the code ourselves, from reliable automated checks and AI review, or from both. If those checks cover the risks of a change, an extra manual review may add very little. Someone still needs to understand what the checks cover and where they fall short.

What I'd change in the workflow

We can give agents clearer requirements and better checks, then use reviews to investigate what those checks couldn't establish. Here are some changes I'd start with:

1. Define what has to be true before asking for code.

For the retry change, agree on which failures are retryable and how many attempts are allowed. Identify the existing policy and decide whether repeating the operation is safe. For saveAll(), define what success means and what should happen if a write fails. Those decisions should come from the requirements and the system we're changing.

Anthropic's January 2026 guide to evaluating agents recommends well-specified tasks and stable test environments for coding agents. My suggestion is to apply that to each change and agree on an observable result, then decide how we'll check it.

2. Check that the tests can catch the bug.

For a bug fix, run the new regression test against the broken code first. Confirm that it fails for the reason you expect, then make the change and run it again. Writing a test first doesn't help much if it already passes before the fix. Keep the reviewed expectations fixed while implementing, and use mutation testing on the changed behavior to look for faults the tests still miss.

For more complicated behavior, ask the agent to propose properties that should always hold, and check those against the requirements. A property-based testing tool can then generate inputs to look for exceptions. For retries, we might check that the number of attempts never exceeds the agreed limit across different failure sequences.

3. Make routine verification automatic and visible.

Put the build, type checks, and relevant tests in CI. Have the agent run the appropriate checks while it works, fix failures, and rerun them at the end.

If there's a UI, giving the agent a browser can be really powerful. Have it run the app and go through the user flow it changed. For a save action, that means submitting the form and reloading the page to check that the change persisted. It should inspect the result, including screenshots when appearance matters. OpenAI's February 2026 write-up describes giving agents browser access so they could reproduce bugs and verify fixes directly. GPT 6 Astra is really good with computer use.

Build the setup that makes this possible. The agent needs a reliable way to start the app, reset test data, and a test account if the flow requires login. Give it access to logs so it can investigate failures. Make checking the changed flow part of finishing the task, and keep a record of what it tried and what happened. If it finds a bug, add a regression test so the next change gets checked for it automatically.

4. Keep changes focused, and review how they affect the rest of the system.

PR size matters a lot, especially now that generating code is so cheap. It's easy to produce a huge PR that takes much longer to understand than it took to generate. Keep PRs small and focused on the problem you're solving, and handle unrelated cleanup separately. That makes reviews easier and gives reviewers a better chance of spotting mistakes. Small, focused PRs also make it easier to trace a bug to the change that introduced it and revert that change without undoing unrelated work. Keeping commits independently testable helps when using git bisect, too.

During review, check how the changed code interacts with its call sites and the rest of the system. The retry example shows why this matters: the new code can work correctly on its own and still duplicate something that already exists elsewhere in the repo.

Passing tests doesn't mean we understand the system we're building. If nobody can explain how the generated code fits into the system or why certain decisions were made, future changes become harder to judge and bugs harder to investigate. That creates a lot of maintenance work too because someone eventually has to reconstruct the understanding we skipped. Agents can do that work too, but when they lack context, they fill the gaps with assumptions unless we correct them.

5. Build an AI reviewer for your codebase, and check that it works.

Every project has its own conventions and patterns. An AI reviewer needs to know what those are and which decisions are intentional. Give it focused rules with examples and pointers to the relevant code. Explain things like which layer owns validation or what counts as a breaking API change. GitHub's guidance on customising code review recommends concrete examples and separate instructions where different parts of the repository need different rules.

If you use greptile you can define custom rules.

This is worth investing in! AI reviews catch real bugs, and we should absolutely take advantage of that. I've seen them catch bugs I honestly don't think I would have found myself. With the newer frontier models in particular (GPT 6 Astra or Fable 5.1), I've been impressed by how persistently they dig through the code and follow up on something that looks wrong.

Build the reviewer up over time. When it flags an intentional pattern, clarify the guidance. When it misses an important check, add a specific rule or a better example. Update or remove instructions as the codebase changes. Keep track of which findings helped and which just created extra work, so you can tell whether those adjustments are improving the reviews.

Try it on past PRs that introduced bugs, using the code and context available at the time without the later fix or incident explanation. Include some correct changes too, and repeat these checks when you change the model or review instructions. A January 2026 account of Datadog's approach describes reconstructing PRs behind past incidents and asking the engineers involved whether the AI's feedback would have helped. That gives us a way to test the reviewer against problems our own team encounters.

6. Invest in the software factory, and let it review before a human does.

Let's be honest, most of us don't write much code by hand anymore. Agents write it, and the system around them decides how much we can trust what comes out. That system deserves the same engineering investment as the product it builds.

Points 3 and 5 are parts of that factory. The investment is in wiring them into a pipeline with a fixed order, so a change has to clear each stage before a human gets pulled in. The agent runs the build, the tests, and the changed user flow. Linters, structural tests, the locked acceptance tests, and a mutation run on the changed behavior reject what they can. End-to-end checks can run on every PR as well. Meticulous records sessions from your development and preview environments and replays them against each PR to show how the change affects user flows. TesterArmy has an agent click through the app based on tests written in plain English and reports back with screenshots and recordings. Then AI reviewers with focused jobs, such as repository conventions, security, or duplicated logic, go through the diff, and a separate step verifies their findings so only confirmed issues reach the author.

Anthropic's Code Review basically works this way. There are several agents that look for bugs in parallel, verify them to filter out false positives, and rank them by severity. Anthropic reports that the share of its internal PRs with substantive review comments went from 16% to 54%. Well, this is a vendor measuring its own product, but I agree with where it draws the line which is:

"It won't approve PRs — that's still a human call"

So human review stays but it just starts later. By the time someone opens the PR, the routine questions should already have answers, and the PR should show which stages ran and what they found. The reviewer can then spend their attention on what the pipeline couldn't establish, like whether the requirement is right and which tradeoffs we're accepting. That's also what makes their approval worth more than a click.

7. Find the root cause and fix the class of bug.

It's easy to throw an agent at a bug report and stop once the reported problem seems to go away. But if similar bugs keep appearing, we need to look for what they have in common. Have the agent reproduce the failure and trace where things first go wrong.

Ask the agent to look for the same bug elsewhere in the codebase. Before changing anything, confirm those cases have the same cause. If the problem comes from shared code, fix it there. Add tests for the original bug and the other affected cases. Where possible, make the code prevent the mistake, for example by checking incoming data before passing it on. Keep unrelated changes out of the fix.

AI code can pass its tests and still make a system worse. If it's hard to understand, unsafe to use, or a pain to maintain, someone eventually has to fix that. Calling it finished and leaving that work to someone else is what I mean by slop.

Preventing that means investing in tests, review, and a shared understanding of the system. AI can help with all of this. Whether we review the code ourselves or rely on verification systems we've built, we're responsible for knowing what those checks establish and what still needs attention.

[Boost]

Dominic Elm — Mon, 16 Mar 2026 08:28:08 +0000

Jamund Ferguson

Mar 15

Code Review Kinda Sucks Now

#discuss #ai #codereview #softwaredevelopment

9 min read

Decorators do not work as you might expect 🤔

Dominic Elm — Tue, 16 Jul 2019 10:05:14 +0000

While working on a library called ngx-template-streams, which in a nutshell allows you to work with events as streams in Angular templates, I have discovered that decorators are not instance-based but rather class-based. I was not aware of this behavior and thought that decorators get applied per class instance. In this blog post, we'll take a closer look at decorators and explore why they behave this way and how we can create instance-based decorators.

Cover photo by Garett Mizunaka on Unsplash

Quick Recap on Decorators

Decorators are great. They allow us to add annotations and a meta-programming syntax for class declarations and members, including properties, accessors, parameters, and methods. In other words, we can use decorators to attach additional responsibility to an object without modifying any other object. Therefore, they are great to compose pieces of functionality in a declarative fashion. That means the decorator design pattern is designed in a way that multiple decorators can be stacked on top of each other, each adding new functionality.

Also, many people consider decorators as a flexible alternative to subclassing. While subclassing adds behavior at compile-time, and therefore affecting all instances, decorators add behavior to individual objects at runtime.

So decorators have gained much popularity, and quite frankly for a reason. They make our code easier to read, test, and maintain. Thus, some of the leading open source projects have adopted the decorator design pattern, including Angular, Inversify or Nest.

Ok, so what's a decorator?

Idan Dardikman summarizes this question wonderfully:

Decorators are just a clean syntax for wrapping a piece of code with a function

TypeScript has experimental support for decorators. However, there is an ECMAScript decorator proposal that has reached stage 2 (draft), so they could eventually land in vanilla JS.

As mentioned earlier, there are different types of decorators. For example, we could attach a decorator to a class:

@Component()
class HeroComponent {}
}

The @Component() is an excellent example for a class decorator, and it's one of the core building blocks in Angular. It attaches additional metadata to the class.

Most likely you'll also encounter some property, method or parameter decorators along the way:

@Component()
class HeroComponent {
  @Input() name: string;

  constructor(@Inject(TOKEN) someDependency: number) {}

  @deprecated
  greet() {
    console.log('Hello there!');      
  }
}

So decorators are quite universal, expressive, and powerful. Now, this blog post is not about explaining decorators in all their details. In this post, we implement a property decorator to explore their behavior, but we won't look at the implementation of other types of decorators. If you want to learn more about decorators in general, I highly recommend the official documentation, this gentle introduction or this fabulous series on a variety of topics related to decorators.

The @Clamp Decorator

It's time for an example to understand the behavior that I mentioned in the beginning. The claim was that decorators are not instance-targeted and only called once per class and usage.

To proof this, we'll implement our own property decorator called Clamp.

To use decorators in TypeScript, we have to enable a compiler option called experimentalDecorators. The best place to do this is the tsconfig.json:

{
  "compilerOptions": {
    "target": "ES5",
    "experimentalDecorators": true
  }
}

We can now create a Clamp decorator that we would apply to properties of type number. Its job is to clamp the property value within a specified upper and lower bounds.

For example, if the lower bound is 10 and the upper bound is 50, then our decorated should clamp a value within those bounds:

clamp(5) // => 10
clamp(100) // => 50

We'll implement this functionality later, but first, let's shift our attention to the property decorator.

A property decorator has the following signature:

type PropertyDecoratorType = (target: any, propertyKey: string | symbol) => void;

It's a plain old function with two parameters. The target is the object that owns the decorated property, and the propertyKey is the name of the decorated property. Now, you might be thinking that the target is the instance of a class, but that's not quite the case. The target is simply the prototype of the class, but more on this in just a moment.

The signature above describes a property decorator, and it's well defined. That means the parameters are fixed, and there's no room for extending the signature. However, our decorator is supposed to be configurable and accept a lower and upper bound. Therefore, we have to use the a factory function. In other words, we enclose the decorator method within another method (factory) that defines all configurable options:

function Clamp(lowerBound: number, upperBound: number) {
  return (target: any, propertyKey: string | symbol) => {
    // logic goes here
    console.log(`@Clamp called on '${String(propertyKey)}' from '${target.constructor.name}'`);
  }
}

Nice, we turned a regular decorator into a decorator factory to unleash even more power. Yay!

Before implementing the logic, let's give it a spin! We'll create a class TestBench and decorate some properties with our homemade @Clamp decorator:

class TestBench {
  @Clamp(10, 20)
  a: number;

  @Clamp(0, 100)
  b: number;
}

That's our simple test bench. Note that we are not creating an instance of the TestBench class. So before we run this code, let's do a little quiz:

Question: What do you expect to happen?

A: Nothing. The decorator doesn't get called because we are not creating an instance of the class; hence, nothing is logged.
B: The decorator factory is called once per class; thus, there will only be one value printed to the console.
C: The factory is called twice, once per property; hence, there will be two values printed to the console.
D: It explodes.

Ok, drum roll... 🥁🥁🥁

Running this code gives us the following output:

@Clamp called on 'a' from 'TestBench'
@Clamp called on 'b' from 'TestBench'

Tada! Wait, what? So it seems that our decorator function is called twice, once per decorated property. This means the solution to the quiz above is C. In case of doubt, here's a live demo:

The question now is, why, why is the decorator method called without us creating an instance of the class.

Exploring decorators under the hood

To find the answer to this question, we have to dive a bit deeper and see what is actually generated by the TypeScript compiler if we use a decorator. You can either run tsc or copy and paste the code into the TypeScript Playground. No matter what we do, we should get the following transpiled code:

"use strict";
var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
    else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
    return c > 3 && r && Object.defineProperty(target, key, r), r;
};
function Clamp(lowerBound, upperBound) {
    return (target, propertyKey) => {
        // logic goes here
        console.log(`@Clamp called on '${String(propertyKey)}' from '${target.constructor.name}'`);
    };
}
class TestBench {}
__decorate([
    Clamp(10, 20)
], TestBench.prototype, "a", void 0);
__decorate([
    Clamp(0, 100)
], TestBench.prototype, "b", void 0);

At first glance this is not easy to grok, especially this somewhat magical __decorate function defined at the top. But this method is pretty important, especially how it's consumed.

So where does __decorate come from and what does it do? This method comes from deep, deep, deep down the compiler and is generated when any type of decorator is used. TypeScript has a helper that produces this code, and it's called decorateHelper. Definitely check out the source code. It's a great learning resource.

Ok, but what does it do? Simply put, it loops over every decorator that is passed in and tries to evaluate them. A detailed explanation is outside the scope of this post. Fortunately, there is an excellent blog post that explains this in-depth.

So let's draw our attention to the bottom of the generated code:

__decorate([
    Clamp(10, 20)
], TestBench.prototype, "a", void 0);
__decorate([
    Clamp(0, 100)
], TestBench.prototype, "b", void 0);

That's where the __decorate function is consumed. Also, we can see that it's called twice, once per decorated property and both get the same target passed in, that is TestBench.prototype. The second argument is the propertyKey and the last argument is a property descriptor. Here, void 0 is used to pass undefined.

The void operator takes an expression, evaluates it and returns undefined. For more information, see the MDN documentation. There is also a great StackOverflow answer that explains the purpose of this tiny operator compared to just using undefined. TL;DR: JavaScript isn't a perfect world, and undefined is not a reserved keyword, meaning that it's just a variable name and we could assign a new value to it.

So the code above is the code that gets generated by the TypeScript compiler, and typically we would load the code in a browser where it gets executed once the file is loaded. In other words, decorators will be applied the moment the classes, in which we use decorators, are loaded. As a result, the decorator (here our property decorator) only has access to the prototype of the class and the property name, but not the instance. This is by design, and it all makes sense, now that we know what gets generated by the compiler.

So far the key takeaway should be that we now know why decorators are not instance-targeted and instead executed when our JavaScript gets loaded in the browser.

It's essential to be aware of this because otherwise, we might experience unexpected behavior. To understand this, we'll have to add logic to our decorator.

The Problem

The fact that decorators are applied when the class is loaded, not when we create instances, is not incorrect and that's actually by design. So what could possibly go wrong?

To find this out, we start off by implementing the actual clamp functionality. So let's create a factory called makeClamp which returns a clamp function with an upper and lower bound. Using a factory function again here makes the functionality more reusable.

function makeClamp(lowerBound: number, upperBound: number) {
  return function clamp(value: number) {
    return Math.max(lowerBound, Math.min(value, upperBound));
  }
}

We can see that this factory returns a clamp method. Here's an example of how we could use this:

const clamp = makeClamp(0, 10);

console.log(clamp(-10)); // => 0
console.log(clamp(0));   // => 0
console.log(clamp(5));   // => 5
console.log(clamp(10));  // => 10
console.log(clamp(20));  // => 10

The examples above should give us a proper understanding of what the decorator is supposed to do. A class property annotated with @Clamp should clip the property value within an inclusive lower and upper bound.

Simply adding this to the decorator function is not enough, because we want the decorator to be operating on an instance and it's supposed to clamp the value of a property every time it's being set.

Let's say we didn't know that the target was only the prototype of a class, so we modify the already existing property on the target using Object.defineProperty. This will allow us, besides other things, to define a getter and setter, which is exactly what we need. Here's what we have to do:

create a desired clamp method using the factory makeClamp.
maintain some internal state used to store the clamped property value.
modify the target property using Object.defineProperty and provide a getter and setter so that we can intercept any modification to the value and run it through our clamp method.

Putting this into code could look like this:

function Clamp(lowerBound: number, upperBound: number) {
  return (target: any, propertyKey: string | symbol) => {
    console.log(`@Clamp called on '${String(propertyKey)}' from '${target.constructor.name}'`);

    // 1. Create clamp method
    const clamp = makeClamp(lowerBound, upperBound);

    // 2. Create internal state variable that holds the clamped value
    let value;

    // 3. Modify target property and provide 'getter' and 'setter'. The 'getter'
    // simply returns the internal state, and the 'setter' will run any new value
    // through 'clamp' and update the internal state.
    Object.defineProperty(target, propertyKey, {
      get() {
        return value;
      },
      set(newValue: any) {
        value = clamp(newValue);
      }
    })
  }
}

Let's also update our test bench, remove one property for simplicity, and create two instances of the test class. Furthermore, we'll set the property to some value:

class TestBench {
  @Clamp(10, 20)
  a: number;
}

const tb1 = new TestBench();
console.log(`Setting 'a' on TB1`)
tb1.a = 30;
console.log(`Value of 'a' on TB1:`, tb1.a);

const tb2 = new TestBench();
console.log(`Value of 'a' on TB2:`, tb2.a);

Running this code will print the following output:

@Clamp called on 'a' from 'TestBench'
Setting 'a' on TB1
Value of 'a' on TB1: 20
Value of 'a' on TB2: 20

Now, this output seems a bit off, doesn't it? We create the first instance tb1 and immediately set property a to 30. This results in the setter to be called, which clamps the value within the specified upper and lower bound. The result should be 20, and that's the case. So far, so good. Then we create another instance tb2 and simply read the property, causing the getter to be called. Somehow this returns 20 even though we haven't set the value on the second instance. Why?

This is what I meant by unexpected behavior, at least if we are not aware of the fact that the target is not the class instance but the prototype. So any modifications on the target will affect every instance because we are globally modifying the prototype of the class. Also, the value that was meant to be an internal state to every decorator is shared across all instances, because they all share the same decorator scope. It is what it is, but for our use case, that's not cool.

Check out this live demo! I highly encourage you to noodle around with the code a bit.

Creating instance-targeted decorators

So what do we do if we want our decorator to be instance-based? We certainly don't want to share state across instances globally.

The solution involves modifying the target property once the decorator is applied, as well as defining a property on the instance with the same property name. In other words, we define a property with a setter on the target prototype that will install a property with the same name, that is propertyKey, on the target instance once it is used for the first time.

Ok, let's take a look at the code. I have added a whole bunch of comments to make it easier to understand what's going on:

function Clamp(lowerBound: number, upperBound: number) {
  return (target: any, propertyKey: string | symbol) => {
    console.log(`@Clamp called on '${String(propertyKey)}' from '${target.constructor.name}'`);

     // Create clamp method
    const clamp = makeClamp(lowerBound, upperBound);

    // Create map to store values associated to a class instance
    const values = new WeakMap();   

    // Define property on the target with only a `setter` because we don't
    // want to read from the prototype but instead from the instance.
    // Once the value of the property is set for the first time we define
    // a property with a `getter` and `setter` on the instance.
    Object.defineProperty(target, propertyKey, {
      set(newValue: any) {
        console.log('set on target');

        // This `setter` gets called once per new instance, and only the 
        // first time we set the value of the target property.

        // Here we have access to the instance `this`, so we define 
        // a property with the same name on the class instance.
        Object.defineProperty(this, propertyKey, {
          get() {
            console.log('get on instance');
            // This `getter` gets called every time we read the instance property.
            // We simply look up the instance in our map and return its value.
            return values.get(this);
          },
          set(newValue: any) {
            console.log('set on instance');
            // This `setter` is called every time we set the value of the 
            // property on the class instance.
            values.set(this, clamp(newValue));
          }
        });

        // Finally we set the value of property on the class instance.
        // This will trigger the `setter` on the instance that we defined above.
        return this[propertyKey] = newValue;
      }
    })
  }
}

Essentially, we are using Object.defineProperty inside Object.defineProperty but with different objects. The first one uses the target which is the class prototype, and the second one uses this which refers to the class instance.

Also, note that we are using a WeakMap at the top of the decorator to store the property value for each instance. A WeakMap is a special kind of Map but the difference is that a WeakMap doesn't prevent an object from being garbage collected even though this object is used as the key in the WeakMap. If you want to learn more, check out this fantastic blog post which explains the differences really well.

Alright, let's give this revised version of our decorator a spin and see if it is really instance-targeted and if it no longer shares state across all instances of the same class. For that, I have slightly updated our test bench and added a few comments:

// When this class gets loaded, the decorator is applied and executed.
// This will define the `setter` for the target property on the prototype
// of this class.
class TestBench {
  @Clamp(10, 20)
  a: number;
}

const tb1 = new TestBench();

// This should return `undefined` because we didn't define a `getter`
// on the target prototype for this property. We only install a `getter`
// once we set the value for the first time.
console.log(`Reading 'a' on TB1`, tb1.a);

// This calls the `setter` for `target.a` and defines a property with 
// a `getter` and `setter` on the class instance for the same property.
tb1.a = 30;

// From this moment on, every time we read the value for this property
// we would call the most inner `getter`.
console.log(`Reading 'a' on TB1`, tb1.a);

// The same applies for updating the value. This will call the `setter`
// that we defined for the property of the class instance.
console.log(`Updating 'a' on TB1`);
tb1.a = 15;

// Creating a new instance doesn't do anything
const tb2 = new TestBench();

// Remember, we have globally defined a getter for `target.a` and because we
// are operating on a new instance, the target setter will be called which
// will set up the property on the new instance with their own `getter`
// and `setter` methods.
console.log(`Setting 'a' on TB2`);
tb2.a = 5;

console.log(`Reading 'a' on TB2:`, tb2.a);

// Remains unmodified because every instance has it's own property defined
// with their own `getter` and `setter`
console.log(`Reading 'a' on TB1:`, tb1.a);

Tada! It seems to be working. We have just implemented our own decorator that works on an instance level rather than being prototype-based. I mean it still involves modifying the prototype, but now every decorator also operates on a single instance, and they are all isolated from one another.

Check out the final solution and definitely play around with the code:

Bonus

The above illustrates a full-blown solution, but while I was writing this blog post Netanel Basal pointed out to me a solution that is much more concise and cleaner. It doesn't require a double call to Object.defineProperty, because he found out that the return value is not ignored, as opposed to what's mentioned in the documentation, and is in fact used as an input for a call to Object.defineProperty.

With that in mind, we can reduce our solution from above to the following, which has the exact same behavior:

function Clamp(lowerBound: number, upperBound: number): any {
  return (target: any, propertyKey: string | symbol) => {
    const clamp = makeClamp(lowerBound, upperBound);

    // We need a unique key here because otherwise we would be
    // calling ourselves, and that results in an infinite loop.
    const key = Symbol();

    // We can return a property descriptor that is used to define 
    // a property on the target given the `propertyKey`.
    return {
      get() {
        // Read the value from the target instance using the
        // unique symbol from above
        return this[key]; 
      },
      set(newValue: any) { 
        // Clamp the value and write it onto the target instance
        // using the unique symbol from above
        this[key] = clamp(newValue);
      }
    }
  }
}

Now, this is pretty clean, isn't it? 🔥

Here's a live demo:

Conclusion

Decorators are class and property-based, meaning they are applied and executed once per decorated property when the class gets loaded. This means the target is not the class instance but the prototype of the class. Any changes made to the target are made globally, and if we try to use the decorator scope to maintain some internal state, that state is being shared across all instances of the same class, and they all use the same decorator scope. This could lead to unexpected behavior.

However, in this article, we have seen a solution that involves a double Object.defineProperty with different targets to make a decorator instance-based.

Hopefully, by now, you have a better understanding of how decorators work and why they behave the way they do.

If you enjoyed this post feel free to give it a thumbs up and let me know if you have any questions or comments!

Special Thanks

I’d like to thank Netanel Basal and Manfred Steyer for reviewing the article and providing valuable feedback. 🙏