오픈 소스에 첫 기여하기 (feat. ESLint)
작년 인턴에서 B2B AI 챗봇을 만들며 오픈소스에 큰 도움을 받았고, 그때부터 나도 많은 사람이 쓰는 프로젝트에 기여해보고 싶다는 목표가 생겼습니다. 그런데 첫 머지는 생각보다 쉽지 않았습니다. next.js에 올린 PR은 중복으로 닫혔고, vercel/ai에 올린 PR은 아직 대기 중입니다. 오픈소스는 리뷰 호흡이 길고 PR도 많이 올라오다 보니, 시작선에 서는 것부터가 도전이었습니다.
그래서 다음 도전지를 고민하다, 마크다운 렌더링과 코드 생성 CLI를 만들며 관심이 생긴 AST 기반 도구 중, 가장 익숙한 ESLint를 골랐습니다. 그리고 오늘, 드디어 올렸던 PR이 머지되면서 첫 기여에 성공했습니다. 🎉
처음 코드베이스를 파악하고 이슈를 찾으면서 어려웠던 부분들, 그리고 수정 방향을 두고 메인테이너와 논의했던 과정을 기록으로 남겨보려고 합니다.
머지된 PR을 거슬러 올라가며, 이슈 찾기
이전에 next.js나 vercel/ai에 도전할 때는 이미 올라온 버그 이슈에서 도전할 수 있어, 살펴볼 파일이 처음부터 정해져 있었습니다. 하지만, ESLint는 이미 올라온 이슈에서 제가 작업할 수 있는 게 거의 없었습니다.
ESLint는 이슈를 올리고 팀이 accepted 라벨을 달아줘야 PR을 올릴 수 있고, 이슈 작성자가 원하면 PR 우선권도 가져가기 때문에 상당 부분이 이미 Assignee가 할당되어 있었습니다. 코드베이스를 직접 보고 이슈를 찾는 것이 가장 빠르게 기여할 수 있는 방법이었습니다.
그럼 어디에서 기여할 부분을 찾을 수 있을까요? 🧐
문서 오타나 CI 개선 같은 기여도 있지만, 저는 실제로 사용되는 기능과 관련한 기여를 하고 싶었습니다. 기능 추가(feat)는 메인테이너와 긴 논의가 필요할 수 있어 첫 기여로는 무겁다 생각해, 버그(fix)를 찾아 수정해보기로 했습니다.
그래서 최근에 외부 기여자가 머지시킨 버그 fix PR들을 먼저 리스트업해 봤습니다. 머지 이력을 보면 이 프로젝트가 어떤 문제를 어떤 방식으로 고쳐왔는지, 팀이 어떤 버그 수정을 유의미하게 받아들이는지 알 수 있을 거라 생각했습니다.
그렇게 PR을 거슬러 올라가다 보니, 반복되는 패턴이 하나 보였습니다.
전역 객체인 척하는 지역 변수
자주 머지된 PR 패턴에 대해 이야기하기 전에, ESLint의 룰(rule)에 대해 먼저 알아봅시다.
변수를 선언만 해두고 아직 쓰지 않았을 때, 에디터가 노란 줄을 그으며 'hello' is defined but never used라고 경고가 뜨는 것을 본적이 있었을 겁니다. 그 경고 하나하나가 ESLint의 룰입니다. 앞서 말한 것을 경고하는 룰의 이름은 no-unused-vars 이고, 이 밖에 정말 다양한 룰이 있으며, 각 룰은 코드를 훑으며 자기가 담당하는 패턴을 검사합니다.
그리고 각 룰은 lib > rules > 룰 이름.js로 하나의 js 파일로 정의되어 있습니다.
그런데 이런 코드를 만나면 어떨까요?
function f(NaN) {
// 이 NaN은 전역 NaN이 아니라 그냥 파라미터 이름(지역 변수)
}이상한 코드지만 유효한 자바스크립트입니다. 문제는 일부 룰이 이 NaN을 전역 NaN으로 착각하고 엉뚱한 경고를 낸다는 것이었습니다. 잘못이 없는 코드에 경고가 뜨는 것, 이게 오탐(false positive)입니다.
머지 이력에서 발견한 반복 패턴이 바로 이것이었습니다. 전역 내장 객체와 같은 이름의 지역 변수가 전역을 가릴 때(shadowed) 룰이 잘못 경고하는 오탐을 고치는 PR이 #20958(use-isnan), #21047(prefer-numeric-literals) 등 여러 건 머지되어 있었습니다.
관련 PR 리스트
섀도잉된 전역(shadowed globals) 오탐을 해결하는 PR로 수정 패턴은 항상 동일했습니다.
수정 방식: sourceCode.isGlobalReference(node) 검증 추가 + 테스트 케이스 추가
- #21047
prefer-numeric-literals - #20958
use-isnan - #21051
no-invalid-regexp - #21050
no-control-regex - #21013
no-extra-boolean-cast, - #21003/#21006
prefer-promise-reject-errors, #20740no-async-promise-executor, #21010no-throw-literal, #21011radix
수정 방식도 항상 같았습니다. 변수가 진짜 전역 객체를 가리키는지 판별해주는 메서드 sourceCode.isGlobalReference()가 아래 PR에서 도입되었는데요.
위 오탐을 고치는 모든 PR들은 위 메서드를 룰의 검증로직에 추가하고, 테스트 케이스를 붙이는 식이었습니다.
그렇다면, 아직 코드베이스에 sourceCode.isGlobalReference()를 통한 검증 로직이 추가 되지 않은 부분이 있지 않을까요?
아직 안 고쳐진 룰 찾기
시작은 단순한 연상이었습니다. sourceCode.isGlobalReference()를 도입한 위 PR에서, 이 메서드를 처음 적용한 룰 중 하나가 no-setter-return이었는데요. 찾아보니, setter에 관한 룰이었습니다. setter를 검사하는 룰이 고쳐졌다면, 짝이 되는 getter 쪽 룰에는 같은 오탐이 남아 있지 않을까 생각해 AI와 함께 찾아보니, getter-return 룰이 대응되는 룰임을 알아냈습니다.
no-setter-return룰: setter는return구문이 있으면 안된다.return구문이 존재하면 린트 오류가 난다.getter-return룰: getter는 반드시 값을return해야한다. 하지 않으면 린트 오류가 난다.
no-setter-return에서 전역 객체인지 판별이 필요했던 케이스는 Object.defineProperty(), Reflect.defineProperty(), Object.defineProperties(), Object.create()와 같이 전역 객체의 정적 메서드를 통해 객체의 setter를 정의하는 상황입니다. 관련 테스트케이스가 존재하여 getter 버전을 만들어 플레이그라운드에서 오탐이 발생하는지 확인해보았습니다.
getter-return에서 발견한 false positive
ESLint 플레이그라운드에서 재현해보면, 두 가지 상황에서 오탐이 발생한다는 것을 알수 있습니다.
1️⃣ 전역 객체로 객체를 정의할 때 디스크립터가 아닌 부분에서 getter가 리턴하지 않는다고 오류가 납니다.
2️⃣ 전역 객체가 아닌 파라미터 변수의 메서드로 실행한 함수에서 getter로 판단하고 있습니다.
getter-return은 eslint:recommended 기본 추천 설정에 포함된 룰입니다. 별다른 설정 없이 ESLint를 쓰는 거의 모든 프로젝트에서 켜져 있으니, 오탐 하나가 그만큼 많은 개발자에게 닿는다는 뜻입니다. 고칠 가치가 충분해 보여 원인을 파보기로 했습니다.
이러한 오류가 발생하는 이유는 getter-return에서는 단순히 전역 변수 이름인지 텍스트로만 비교하고, get 메서드가 있는 부분의 인자 위치도 고려하지 않기 때문입니다.
getter-return 버그 원인 찾기
no-setter-return과 나란히 놓고 보기
원인은 두 룰을 비교하니 명확해졌습니다. no-setter-return은 룰 안에서 정의된 헬퍼 함수 두 개를 통해, 헬퍼로 세 가지를 검증합니다.
1️⃣ isArgumentOfGlobalMethodCall() 유틸 함수
// lib/rules/no-setter-return.js#L28-L104
function isArgumentOfGlobalMethodCall(node, sourceCode, objectName, methodName, index) {
const callNode = node.parent;
return (
callNode.type === "CallExpression" &&
// ⓐ 인자 위치 확인 : 호출된 함수에서 몇번째 인자인지 정확하기 확인
callNode.arguments[index] === node &&
// ⓑ 이름 확인 (텍스트): 호출된 함수가 기대하는 객체와 메서드 이름(예: Object.defineProperty)인지 텍스트로 확인
astUtils.isSpecificMemberAccess(
callNode.callee, objectName, methodName,
) &&
// ⓒ 전역인지 확인(의미): 호출된 함수가 전역 객체로부터 호출된 건지 확인
sourceCode.isGlobalReference(
astUtils.skipChainExpression(callNode.callee).object,
)
);
}2️⃣ isPropertyDescriptor() 유틸 함수
상세 코드
// lib/rules/no-setter-return.js
function isPropertyDescriptor(node, sourceCode) {
// 모양 A: Object.defineProperty(obj, "foo", node)
// 디스크립터가 3번째 인자에 직접 놓임
if (
isArgumentOfGlobalMethodCall(node, sourceCode, "Object", "defineProperty", 2) ||
isArgumentOfGlobalMethodCall(node, sourceCode, "Reflect", "defineProperty", 2)
) {
return true;
}
// 모양 B: Object.create(proto, { foo: node })
// 디스크립터가 맵 객체 안의 "값" 자리에 놓임
const parent = node.parent;
return (
parent.type === "Property" &&
parent.value === node &&
parent.parent.type === "ObjectExpression" &&
(isArgumentOfGlobalMethodCall(parent.parent, sourceCode, "Object", "create", 1) ||
isArgumentOfGlobalMethodCall(parent.parent, sourceCode, "Object", "defineProperties", 1))
);
}위 헬퍼를 isPropertyDescriptor()라는 함수가 감싸서 실제 판정에 씁니다. 디스크립터가 놓이는 자리는 API마다 조금 다른데, 아래 와같습니다.
Object.defineProperty(o, k, node): 3번째 인자(index 2)인지 검사Reflect.defineProperty(o, k, node): 3번째 인자(index 2)인지 검사Object.create(proto, {k: node}): 노드의 조부모 객체가 2번째 인자(index 1)인지 검사Object.defineProperties(o, {k: node}): 노드의 조부모 객체가 2번째 인자(index 1)인지 검사
getter-return의 구현
반면 getter-return은 전역 객체 이름을 텍스트로만 비교하고 있었습니다.
// lib/rules/getter-return.js
function isGetter(node) {
const parent = node.parent;
// ... (중략)
// Object.defineProperty() or Reflect.defineProperty()
if (parent.parent.parent.type === "CallExpression") {
const callNode = parent.parent.parent.callee;
if (
// ⓑ 이름 확인뿐, ⓐ 몇 번째 인자인지도 ⓒ 진짜 전역인지도 안 봄
astUtils.isSpecificMemberAccess(callNode, "Object", "defineProperty") ||
astUtils.isSpecificMemberAccess(callNode, "Reflect", "defineProperty")
) {
return true;
}
}
// Object.defineProperties() or Object.create()
// ... 여기도 마찬가지로 이름 비교만
}ⓐ 인자 위치도 ⓒ 전역 여부도 확인하지 않으니 두 케이스 모두에서 오탐이 나는 것이었죠. 즉 이 버그는 no-setter-return의 로직을 그대로 옮겨오면 해결되는 문제였습니다.
이슈 올리고, 메인테이너와 수정 방향에 대해 논의하기
버그 이해는 어렵지 않았습니다. 다만, PR 단위와 수정 방식에 대한 고민이 들었습니다.
1️⃣ PR 단위: 일단 getter-return의 두 가지 오탐(ⓐ 인자 위치 X, ⓒ 전역 여부 X)은 엄밀히 다지면, 종류가 다릅니다. 두 개의 PR을 열어야할지 하나의 PR로 작업을 해도 괜찮을지 고민이 되었습니다.
2️⃣ 수정 방식: 사실상 no-setter-return의 내부 유틸 함수의 로직을 그대로 적용하는 것인데, 로직을 복사해 그대로 적용하는 것이 맞을지 유틸을 공통 유틸로 올려 룰에 적용할지 고민이 되었습니다. 공통 유틸이 되면, 룰이 해당 유틸 함수에 의존하게 되기 때문에, 유틸 함수에 변경사항이 생기면 변경 범위가 커질 것이라 생각되어 조심스러웠습니다.
다만, 좀 더 찾아보니, accessor-pairs도 비슷한 로직이 존재했습니다.
accessor-pairs룰: getter와 setter가 짝을 이루는지 검사하는 룰. setter가 존재하면 반드시 getter가 존재해야한다.
그 반대의 경우도 옵션을getWithoutSet: true로 주면 검사할 수 있다. (기본은false)
정리하자면, 아래 룰에 객체 검사 시, ⓐ 인자 위치, ⓑ 텍스트 이름 확인, ⓒ 전역 객체 여부 검증 로직이 존재하거나 필요로 합니다. 이렇게 세 개의 룰에서 필요한 거라면 유틸로 빼는 것도 나쁘지 않아보입니다.
| 규칙(rule) | ⓐ 인자 위치 | ⓑ 이름 확인 | ⓒ 전역 검증 | 결과 |
|---|---|---|---|---|
no-setter-return | O | O | O | 정상 |
accessor-pairs | O | O | X | 전역 객체 오탐 |
getter-return | X | O | X | 오탐 |
제가 생각한 방식과 고민사항을 잘 작성하여 아래처럼 이슈를 올렸습니다.
- 재현: 두 케이스의 재현 코드와 ESLint 플레이그라운드 링크
- 근거: 같은 케이스를
no-setter-return은 올바르게 처리한다는 코드 비교 - 제안: 두 오탐은 같은 검사에서 나오므로 PR 하나로 함께 수정, 방식은
no-setter-return의 검증(인자 위치 + 전역 참조)을 그대로 적용 - 질문:
accessor-pairs에도 같은 로직이 있어 이대로면 세 번째 중복이 생기는데,ast-utils로 추출해 재사용할지, 일단 복제하고 추출은 후속으로 넘길지
몇 시간 뒤, DMartens 메인테이너 분으로부터 아래와 같은 답변을 받을 수 있었습니다. 공통 유틸로 해당 로직을 옮기고, 모두 세 가지 룰에 적용하는 것을 하나의 pr에서 작업하면 된다고 말씀해주셨습니다.
PR 작업하고 검토받기
PR은 두 단계로 작업했습니다.
1. 오탐을 잡아내는 테스트부터
로직을 변경하기 전에 코드베이스 테스트 내에서 직접 오탐이 발생하는지 체크하고, 수정 후 잘 통과하는지 검사하면 좋을 것 같다 생각했습니다. 그래서 아래 테스트 케이스를 추가했습니다.
getter-return: ⓐ 인자 위치가 다른 케이스, ⓒ 전역 객체가 아닌 케이스 추가accessor-pairs: ⓐ 인자 위치가 다른 케이스(이미 통과하는 테스트지만 케이스가 없어 보강), ⓒ 전역 객체가 아닌 케이스 추가
no-setter-return의 기존 테스트를 참고해 getter 버전으로 바꿔 valid 섹션에 추가했습니다.
2. 헬퍼를 공통 유틸로 추출
no-setter-return의 헬퍼 두 개를 ast-utils로 옮기고, 세 룰이 모두 astUtils.isPropertyDescriptor()를 쓰도록 바꿨습니다.
이때, 이전에 읽었던 📖 [리팩터링 2판] 6.1 함수 추출하기에 나온 방식대로 작은 단위로 나눠서 작업하고 커밋해봤습니다.
추출한 메서드의 노출 범위도 고민했습니다.isArgumentOfGlobalMethodCall() 메서드는 내부에서만 쓰이므로 jsDoc에 @private로 표시하고 export하지 않았고, isPropertyDescriptor만 export했습니다.
첫 리뷰는 당일에 왔습니다. 메인테이너의 "LGTM, thanks!"와 함께 다른 리뷰어를 위해 며칠 열어두겠다는 코멘트였습니다.
이 후, 다른 리뷰어의 요청이 하나 들어왔습니다. 헬퍼가 이제 export되는 공용 유틸이 되었으니, ast-utils 자체의 단위 테스트도 추가해달라는 것이었습니다.
단위 테스트는 각 룰의 테스트와 약간 다르게 작성되어 있어서, 처음 파악하는데 시간이 좀 걸렸습니다.
기존 ast-utils의 테스트 방식을 조사해보니, 크게 세 가지 방법이 있었습니다.
- 소스 코드 string을
espree.parse()로 바로 파싱해서 넣는다.
espree.parse()와new SourceCode(code, ast)로 ast와 소스코드 내용을 모두 확인해서 테스트한다.
linter.verify()를 활용해, 임시 plugin rule을 실행시키는 방법으로 테스트한다.
직접 순회를 하기 때문에 스코프나 노드의 parent도 가져올 수 있다.
mustCall이란?테스트 케이스를 실행할때 최소한 한번은 linter.verify()가 실행되는지 체크합니다.
이런 체크를 하는 이유는, 만약에 rule이 한 번도 실행되지 않는다면 에러가 있어도 실행이 되지 않아 에러가 발생하지 않을 수 있는데 이러한 상황을 막아줍니다.
주의점은 룰이 테스트 케이스 당 한번은 실행이 되는게 정상 케이스인지 체크해야합니다.
다시 돌아와, 우리 isPropertyDescriptor()를 살펴보면, 이 메서드 실행시에 parent를 타고 올라가 검사를 해야하고, 스코프 분석도 필요해 인자로 넘어온 노드만 파싱해서 테스트하기엔 무리가 있습니다.
linter.verify()가 적합하기 때문에, 해당 메서드를 실행하는 assertResult 유틸 함수를 만들어 테스트를 진행했습니다.isPropertyDescriptor() 메서드 안에서 인자로 들어오는 노드가 항상 객체 표현식일때만을 강제하는 것은 아니지만, 지금 이 메서드를 사용하는 모든 곳에서 객체 표현식인지 확인하고 이 메서드를 실행하기 때문에, 이 메서드를 ObjectExpression 노드가 넘어올때마다 실행되도록 하는 게 합당해보입니다.
따라서 모든 ObjectExpression 노드가 넘어올때마다 isPropertyDescriptor()가 실행되도록 하는 임시 plugin 룰을 만들어 linter.verify()로 실행하고 판정 결과를 순회 순서대로 배열로 만들어 검증하는 방식을 택했습니다.
// tests/lib/rules/utils/ast-utils.js
function assertResult(code, expected, languageOptions = {}) {
const results = [];
linter.verify(code, {
languageOptions,
plugins: {
test: {
rules: {
checker: {
create: mustCall(context => ({
ObjectExpression: mustCall(node => {
results.push(
astUtils.isPropertyDescriptor(
node,
context.sourceCode,
),
);
}),
})),
},
},
},
},
rules: { "test/checker": "error" },
});
assert.deepStrictEqual(results, expected);
}
// 단위 테스트 작성
}
테스트가 잘 통과하네요! 🙂
첫 머지 성공 🎉
오늘 새벽쯤에 머지가 됐다고 알림이 떴었는데, 잘 마무리 할 수 있어 너무 좋았습니다! 😊 연습삼아 아주 간단한 ci나 테스트 보강 관련 PR 머지는 최근에 하긴 했지만, 직접적인 기능과 관련하여 유의미한 변경을 작업해 기여한 건 처음이라 의미가 더 깊었던 것 같습니다.
앞으로도 ESlint에 대해 깊게 알아보면서, 기능 구현(feat)에 도전해볼까 합니다. 그리고 typescript-eslint 쪽은 아직 도전하지 않았는데, 이쪽 레포도 살펴보면서, 타입은 어떻게 다루는지 탐구해보려고 합니다.