Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: blog: Code review nit to ecosystem improvements #629

Merged
merged 32 commits into from
Oct 2, 2024
Merged
Changes from 2 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
ebd4167
feat: blog: Code review nit to ecosystem improvements
captbaritone Sep 17, 2024
a0c7e0b
Delete yarn.lock
captbaritone Sep 17, 2024
5c347d1
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
ba3929f
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
8a00087
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
46971ce
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
9830d83
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
42718fd
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
9c4dc03
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
2569866
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
118c418
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
f377e34
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
52172f4
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
c5881a1
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
e64feba
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
b8a991f
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
1469e6a
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
595908f
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
cb0089e
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
c50ae82
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
97a36f2
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
6f71fd1
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
86daf75
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
f134f26
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
c9d246d
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
be904c9
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
d0d3500
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
dbf625e
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Sep 19, 2024
d59b83b
Edits, rule examples, new conclusions
captbaritone Oct 2, 2024
a2c4104
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Oct 2, 2024
97486cd
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Oct 2, 2024
5e85697
Update src/content/drafts/code-review-nit-to-ecosystem-improvements.md
captbaritone Oct 2, 2024
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
60 changes: 60 additions & 0 deletions src/content/drafts/code-review-nit-to-ecosystem-improvements.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
---
layout: post
title: "Code review nit to ecosystem improvements"
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
teaser: "Reflecting on the power of lint rules"
Copy link
Member

Choose a reason for hiding this comment

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

A little more oomph:

Suggested change
teaser: "Reflecting on the power of lint rules"
teaser: "How implementing my first ESLint rule led to changes in how people write JavaScript"

Copy link
Contributor Author

@captbaritone captbaritone Sep 19, 2024

Choose a reason for hiding this comment

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

Not my first ESLint rule, but my first core ESLint rule. What do you think about:

How implementing my first core ESLint rule led to changes in how people write JavaScript

Or

How implementing an ESLint rule led to changes in how people write JavaScript

Copy link
Member

Choose a reason for hiding this comment

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

I like the second one. 👍

authors:
- captbaritone
categories:
- Case Studies
tags:
- Guest Post
- Rules
---
Four years ago, doing code review at work, I was surprised that [Flow](https://flow.org/) had not warned about a null check that had become unnecessary. This month [TypeScript 5.6](https://devblogs.microsoft.com/typescript/announcing-typescript-5-6/#disallowed-nullish-and-truthy-checks1) released with validation rules that disallows nullish and truthy checks which uncovered nearly 100 existing bugs in the top 800 TypeScript repos on GitHub.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved

The two events are connected, because that moment in code review four years ago lead me to write [a lint rule](https://eslint.org/blog/2022/07/interesting-bugs-caught-by-no-constant-binary-expression/) which was part of the inspiration for the TypeScript features.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Member

Choose a reason for hiding this comment

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

Before going into the timeline, I think it would be helpful to introduce the rule and show an example of an error it detects. So maybe a "What does the no-constant-binary-expression rule do?" section.

Given the protracted timeline and the many intermediate steps I thought it would be interesting to reflect on what lead to this observation in code review snowballing into what I feel is a significant impact to a large number of developers, and why I think the snowball could continue to grow.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved

First a timeline:
captbaritone marked this conversation as resolved.
Show resolved Hide resolved

## Timeline

* May 2020: I was reviewing a diff/PR at work that made a nullable value non-nullable and noticed that the author had left behind an if condition that handled the now impossible case where the value was null. I started to wonder why Flow hand’t automatically pointed that out.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* May 2020: I made a post in an internal group asking the Flow team about it. The answer was that Flow, like TypeScript, is not sound. For example `arr[x]` is typed as non-nullable but might actually be undefined at runtime. Flow had implemented these checks in the past, but they caused major issues by telling people their null checks were safe to remove when they were not, so they removed the checks. [Brad Zacher](https://zacher.com.au/) happened to see the post and chimed in with an observation a syntactic approach, rather than a type based one, could be safe, even if it were less powerful.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* Aug. 2020: I implemented the syntactic validation approach as an internal ESLint rule, and realized it generalized beyond null checks to all constant comparisons. I ran it in Meta’s mono-repo and found it identified several hundred existing bugs.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* Oct. 2020: Brad Zacher suggested I propose it as a new core rule in ESLint. [I did](https://github.com/eslint/eslint/issues/13752), and they [liked the idea](https://github.com/eslint/eslint/issues/13752#issuecomment-729125654).
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* Nov. 2021-Apr. 2022: [Rewrote it for open source](https://github.com/eslint/eslint/pull/15296), which took a surprising amount of effort due to different positions on things like JSX and style.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* July 2022: Decided I wanted to write a blog post about it. The ESLint team was redoing their site at the time and looking for more blog content, so they asked if I wanted to write the [post on the official blog](https://eslint.org/blog/2022/07/interesting-bugs-caught-by-no-constant-binary-expression/).
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* Nov. 2023: The blog post made the [front page of Hacker News](https://news.ycombinator.com/item?id=38196644). The post was likely shared by someone who read a [Tweet](https://twitter.com/captbaritone/status/1722290945633443973) I posted sharing that it was going to be enabled by default in ESLint 9.0.0
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* April 2024: Rule finally ships as enabled by default in [ESLint 9.0.0](https://eslint.org/blog/2024/04/eslint-v9.0.0-released/). It had to wait for a new major version since adding default rule is a breaking change.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* Jul. 2024: TypeScript team, looking to expand upon the idea of checking for a specific type of constant condition, found the blog post and code and were able to expand the set of validations they performed to include most of the checks we performed. They found similar results. [94 real errors in the top 800 TS repos](https://github.com/microsoft/TypeScript/pull/59217#issuecomment-2221372781). The success of the rule in ESLint gave them confidence to enable the check by default.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* Sept. 2024: The validation ships in [TypeScript 5.6](https://devblogs.microsoft.com/typescript/announcing-typescript-5-6/#disallowed-nullish-and-truthy-checks1).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Sept. 2024: The validation ships in [TypeScript 5.6](https://devblogs.microsoft.com/typescript/announcing-typescript-5-6/#disallowed-nullish-and-truthy-checks1).
* **September 2024:** [TypeScript 5.6](https://devblogs.microsoft.com/typescript/announcing-typescript-5-6/#disallowed-nullish-and-truthy-checks1) ships constant binary expression validations.


## Reflections

Upon reflection, there were a number of key factors that contributed to this small observation in code review helping spur a meaningful ecosystem wide improvement:

* Meta’s internal culture empowered me, a random engineer, to have a direct conversation with members of the Flow team, in a venue where Brad Zacher, an ESLint expert, could happen upon the conversation and chime in.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* Meta’s mono-repo gave me direct access to a massive codebase which let me easily run early drafts of the rule to assess how impactful the validation would be on a vast amount of real code.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* Meta’s culture of autonomy game me the freedom to take on a side-quest of writing this rule despite it not being part of my team’s responsibility.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* ESLint’s pluggable architecture empowering me to write my own rule and easily deploy it across the whole company without needing to convince any gate keepers.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* The ESLint team’s openness to, and active support of, a new contributor adding a new core rule despite a [2020 policy](https://eslint.org/docs/latest/contribute/propose-new-rule) of only accepting new rules if they relate to new language features.
* Active communication about the work initiated by me, and amplified by the ESLint team, in the form of a blog post and Tweets. These allowed the TypeScript team to connect the dots between a more specific request they got about disallowing `if (/regex/)`, and the broader idea of detecting constant conditions.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* Having a somewhat obsessive personality which wasn’t satisfied with just pointing out a useless null check in code review, but saw that a fundamental solution to that class of problem was possible and wasn’t satisfied until that solution was enabled not just for me, my team or my company, but for the broader ecosystem.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved

## What’s Next

It’s taken four years for the ripple of Brad’s initial observation on that internal post to reach this point, but I think the ideas here have the potential to resonate even further:

* TypeScript and Flow could internally track types which they happen to know are sound, and opportunistically report errors based on that data, allowing checks like this to be performed in many many more cases.
* Other unsound languages which previously avoided reporting unnecessary checks for the same reason Flow did, could use this same approach to catch logic errors.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
* More broadly, dead code elimination is a well understood area of compiler design with many codified techniques and approaches. However, they are nearly always applied as optimizations in the compiler backend. I suspect many of these same dead code elimination techniques could be brought to the front-end of the compiler and used to detect and report bugs.

## Conclusion

Solving problems fundamentally requires the combined insights of many people, the persistence of stubborn individuals, active communication, a community that learns from eachother, and often a lot of patience. But if you can make it happen, fundamental solutions scale really well. They apply broadly, can be adapted into other tools and domains, and improve the state of the world not just for developers but to the users those developers serve.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved

---

Thanks to [Brad Zacher](https://zacher.com.au/) for his initial key observation and ongoing encouragement, to [Nicholas C. Zakas](https://humanwhocodes.com/blog/) and [Milos Djermanovic](https://github.com/mdjermanovic) for significant contributions to the rule during code review, and to [Ryan Cavanaugh](https://twitter.com/SeaRyanC) for bringing these same types of validation to the TypeScript ecosystem.
captbaritone marked this conversation as resolved.
Show resolved Hide resolved
Loading