feat: blog: Code review nit to ecosystem improvements #629
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for new-eslint ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for hi-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ja-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for zh-hans-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for fr-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for es-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for pt-br-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for de-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for es-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
amareshsm
reviewed
Sep 18, 2024
src/content/drafts/code-review-nit-to-ecosystem-improvements.md
Outdated
Show resolved
Hide resolved
amareshsm
reviewed
Sep 18, 2024
src/content/drafts/code-review-nit-to-ecosystem-improvements.md
Outdated
Show resolved
Hide resolved
amareshsm
reviewed
Sep 18, 2024
src/content/drafts/code-review-nit-to-ecosystem-improvements.md
Outdated
Show resolved
Hide resolved
amareshsm
reviewed
Sep 18, 2024
src/content/drafts/code-review-nit-to-ecosystem-improvements.md
Outdated
Show resolved
Hide resolved
amareshsm
reviewed
Sep 18, 2024
src/content/drafts/code-review-nit-to-ecosystem-improvements.md
Outdated
Show resolved
Hide resolved
amareshsm
reviewed
Sep 18, 2024
src/content/drafts/code-review-nit-to-ecosystem-improvements.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Amaresh S M <[email protected]>
Co-authored-by: Amaresh S M <[email protected]>
Co-authored-by: Amaresh S M <[email protected]>
Co-authored-by: Amaresh S M <[email protected]>
Co-authored-by: Amaresh S M <[email protected]>
Co-authored-by: Amaresh S M <[email protected]>
nzakas
reviewed
Sep 19, 2024
There was a problem hiding this comment.
Thanks for putting this together. I feel like this is a great story that a lot of folks will enjoy. I left some suggestions for cleaning up grammar and formatting.
I also noted that I think it would be helpful to briefly explain what the rule does and the type of errors it picks up so folks don't have to click a link to get that context.
And I also left a note on the conclusion, which I think can be expanded a bit to review more of the specifics of this story.
| @@ -0,0 +1,60 @@ | |||
| --- | |||
| layout: post | |||
| title: "Code review nit to ecosystem improvements" | |||
There was a problem hiding this comment.
Let's be more specific with the title:
Suggested change
| title: "Code review nit to ecosystem improvements" | |
| title: "no-unused-binary-expressions: From code review nit to ecosystem improvements" |
| --- | ||
| layout: post | ||
| title: "Code review nit to ecosystem improvements" | ||
| teaser: "Reflecting on the power of lint rules" |
There was a problem hiding this comment.
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" |
| --- | ||
| Four years ago, while doing a 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. | ||
|
|
||
| The two events are connected, because that moment in code review four years ago led 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. |
There was a problem hiding this comment.
Suggested change
| The two events are connected, because that moment in code review four years ago led 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. | |
| The two events are connected, because that moment in code review four years ago led me to write [the `no-unused-binary-expressions` rule](https://eslint.org/blog/2022/07/interesting-bugs-caught-by-no-constant-binary-expression/) which helped inspire the TypeScript features. |
|
|
||
| The two events are connected, because that moment in code review four years ago led 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. | ||
|
|
||
| Given the protracted timeline and the many intermediate steps I thought it would be interesting to reflect on what led 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. |
There was a problem hiding this comment.
This is a run-on sentence. I've taken a stab at breaking it up while trying to keep the original meaning.
Suggested change
| Given the protracted timeline and the many intermediate steps I thought it would be interesting to reflect on what led 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. | |
| Given the protracted timeline and the many intermediate steps, I thought it would be interesting to reflect on how we got here. How did this observation in one code review snowball into a significant positive impact to developers? And why does this snowball continue to grow? |
|
|
||
| Given the protracted timeline and the many intermediate steps I thought it would be interesting to reflect on what led 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. | ||
|
|
||
| First a timeline: |
There was a problem hiding this comment.
A little more of an intro here would be helpful.
Suggested change
| First a timeline: | |
| To answer these questions, it helps to review the timeline of events. |
| * 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. | ||
| * 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. | ||
| * 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. |
There was a problem hiding this comment.
Suggested change
| * 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. | |
| * My having a somewhat obsessive personality that wasn’t satisfied with just pointing out a useless null check in code review. Instead, I saw that a fundamental solution to that class of problem was possible, and I wasn’t satisfied until that solution was enabled not just for me, my team, or my company, but for the broader ecosystem. |
| Four years ago, while doing a 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. | ||
|
|
||
| The two events are connected, because that moment in code review four years ago led 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. | ||
|
|
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
Suggested change
| * Other unsound languages which previously avoided reporting unnecessary checks for the same reason Flow did, could use this same approach to catch logic errors. | |
| * Other unsound languages which previously avoided reporting unnecessary checks for the same reason Flow did could use this same approach to catch logic errors. |
|
|
||
| --- | ||
|
|
||
| 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. |
There was a problem hiding this comment.
Suggested change
| 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. | |
| Thanks to [Brad Zacher](https://zacher.com.au/) for his initial key observation and ongoing encouragement, to [Nicholas C. Zakas](https://humanwhocodes.com/) 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. |
|
|
||
| ## Conclusion | ||
|
|
||
| Solving problems fundamentally requires the combined insights of many people, the persistence of stubborn individuals, active communication, a community that learns from each other, 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. |
There was a problem hiding this comment.
I'd like to see this conclusion expanded to talk a bit about the specifics of this story: how a group of developers discovered something that would be generally useful inside a company (for-profit), then used an open source (not-for-profit) project to spread that knowledge independently, ultimately reaching another company (for-profit), that then spread into another open source project (not-for-profit?), to reach an even larger audience.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prerequisites checklist
What is the purpose of this pull request?
Add a blog post reflecting on how ESLint helped a small nit I noticed in code review grow into a fundamental ecosystem improvement shipping in the newest version of TypeScript.
What changes did you make? (Give an overview)
Added a blog post
Related Issues
I previously submitted a blog post about this rule: https://github.com/eslint/eslint.org/pull/240/files
Is there anything you'd like reviewers to focus on?