커스텀 ESLint 규칙으로 동일한 리뷰 코멘트를 줄여보자
안녕하세요. 크리에이트립(Creatrip)에서 프론트엔드 개발을 하고 있는 서종학이라고 합니다.
저희 기술 블로그에 쓰는 첫 글이라서 그런지 매우 떨리네요!
오늘은 프론트엔드 파트에서 반복되는 코드 리뷰 내용을 줄이고 자동화 하기 위해 커스텀 ESLint rule을 만들어 사용하는 과정을 소개하려고 합니다.
문제: 코드 리뷰의 반복되는 코멘트
여러분은 하루에 얼마정도의 시간을 코드 리뷰에 사용하시나요?
저희 파트에서는 보통 구성원들의 하루 업무시간 중 최소 1시간 이상을 코드 리뷰에 할애하고 있습니다. 코드 리뷰를 통해 코드 퀄리티의 유지와 구성원들의 지식 공유를 통한 성장을 목표로 하고 있는 만큼, 각 PR에 최소 2명의 리뷰어를 권장하며 활발한 코드리뷰를 진행하고 있어요.
혹시 코드 리뷰 과정에서 발생하는 컨벤션이나 오탈자 등, 코드의 형태에 대한 반복된 코멘트로 인해 많은 시간을 사용해 본 경험이 있으신가요?
저희 프론트엔드 파트의 코드 리뷰에서는 배열 인덱스 접근시 브라켓 표기법 대신 Array.prototype.at을 쓰는 것을 권장하는 코멘트가 이에 해당했어요.
브라켓 표기법으로 배열의 요소에 접근했을 경우, 타입스크립트에서는 기본적으로 해당 배열의 요소로 타입이 추론됩니다. 하지만 만약 런타임에 해당 배열의 인덱스에 값이 존재하지 않으면 해당 코드는 undefined를 반환하고 이는 예상치 못한 참조 에러를 일으킬 수 있습니다.
const array = [];
const num = array[0]; // number가 추론되지만 실제로는 undefined를 반환합니다.타입스크립트 4.1 부터는 이러한 위험요소를 줄일 수 있는 noUncheckedIndexedAccess flag를 제공하고 있습니다. 하지만 저희 프로덕트에는 해당 flag가 적용되지 않은 상태였어요.
해당 flag의 적용 범위가 배열에 국한되지 않아 고려해야 할 사이드 이펙트도 있었기 때문에 일단 브라켓 표기법을 통한 배열 요소 접근 코드를 더 이상 늘리지 않으며 점진적으로 코드베이스를 개선하기로 했습니다. 또한 코드 리뷰에서 해당 내용에 대해 반복적으로 소통하고 수정하는 비용을 줄여야 할 필요가 있었습니다.
해결 방안: 정적 코드 분석
프론트엔드 파트에서는 반복되는 컨벤션 리뷰 코멘트를 줄이기 위해 커스텀 ESLint rule을 만들어 사용하고 있어요. 맞춤 ESLint rule을 통해 파트 내에서 합의된 코드 형태에 대한 규칙을 자동으로 검사하면 코드 작성 시점에서의 피드백이 이뤄지기 때문에 코드 리뷰어의 시간도 아껴줄 수 있습니다.
저희가 자체적으로 만들고 관리하는 ESLint rule은 @creatrip/eslint-plugin 라는 모노레포 내부의 패키지로 정의되어 있는데요, 이렇게 생성한 패키지는 eslint 설정에서 plugins: [ '@creatrip', ...] 을 통해 간단하게 사용할 수 있습니다.
브라켓 표기법을 통한 인덱스 접근시 에러를 내고 at() 사용을 권장하는 규칙을 추가해서 사용한다면 더 이상 코드 리뷰시 해당 부분에 대한 코멘트를 남기지 않아도 되겠네요!
맞춤형 ESLint rule 만들기
이제 본격적으로 ESLint rule을 만들어 보겠습니다.
저희는 ESLint 규칙을 만들기 위해서 @typescript-eslint/utils 를 사용하고 있는데요, 사용법이 쉽고 간편할 뿐만 아니라 여러 유틸 함수들도 제공하고 있어 AST 파싱에도 편리함을 더해주는 좋은 라이브러리입니다.
먼저 옵션도 없고 메타 데이터도 없는 간단한 룰의 명세를 작성했어요.
import { ESLintUtils } from '@typescript-eslint/utils';
const PREFER_AT = 'PREFER_AT';
export const PreferAtRule = ESLintUtils.RuleCreator.withoutDocs({
meta: {
schema: [],
type: 'suggestion',
docs: {
description: '크리에이트립 배열 인덱스 접근 규칙',
recommended: 'error',
},
messages: {
[PREFER_AT]:
'[크리에이트립 Eslint 규칙]\\n배열 인덱스 접근시 안전한 타입으로 추론되게끔 Array.prototype.at을 사용해주세요.',
},
},
defaultOptions: [],
create(){}
})ESLint rule은 ESTree라고 불리는 ECMAScript의 AST를 순회하며 동작합니다. 저희는 그 중에서 MemberExpression 노드를 사용하여 배열의 인덱스 접근을 감지하려고 해요.
MemberExpression 노드의 인터페이스를 살펴보면 boolean 타입의 computed 프로퍼티를 통해 브라켓을 통한 속성 접근(a[b])인지 정적 멤버 표현식(a.b)인지 알 수 있어요.
그 다음에는 Array.prototype.at 으로 대체할 수 있는 유즈케이스인지 확인을 하는 일이 필요할 것 같습니다.
MemberExpression 에서 브라켓을 통한 접근에 사용되는 인자(a[b] 에서의 b)가 number에 해당한다면 해당 객체가 배열이라고 다소 러프하게 가정할 수 있지 않을까요? 이 경우 a.at(b)을 사용하는 것을 권장해주면 좋을 것 같아요.
위에서 생각한 로직을 실제 코드로 옮겨볼 차례입니다! 먼저 create 메소드의 반환값으로 어떤 AST 노드를 순회할지, 순회하면서 어떤 함수를 동작시킬지를 선언해야 해요.
import { MemberExpression } from '@typescript-eslint/types/dist/generated/ast-spec';
//...
create(context) {
return {
MemberExpression(node: MemberExpression) {
/**
* computed가 false인 경우에는 a[b]과 같은 형태가 아니므로 무시합니다.
* https://github.com/estree/estree/blob/master/es5.md#memberexpression
*/
if (!node.computed) {
return;
}
/** node.property는 브라켓 내부의 노드입니다. (a[b] 에서 b에 해당) */
const indexNode = node.property;
}
}
}
//...여기서 잠깐!
node의 property(a[b] 에서 b 에 해당하는)를 indexNode라고 할 수 있을까요? 우리는 at() 을 사용할 수 있는 케이스를 찾고 있기 때문에, node.property가 number가 맞는지 확인하는 절차가 필요합니다.
이를 위해 isIndexAccess 메소드를 만들어 볼게요.
//...
create(context) {
if (
!context.parserServices ||
!context.parserServices.program ||
!context.parserServices.esTreeNodeToTSNodeMap
) {
throw new Error(
`이 룰은 parserServices 생성이 필요합니다. eslint 설정에서 @typescript-eslint/parser가 제공되었는지 확인해 보세요.`,
);
}
const parserServices = context.parserServices;
const typeChecker = parserServices.program.getTypeChecker();
function isIndexAccess(node: Expression) {
/**
* getStaticValues는 노드 컨텍스트 내부의 참조를 통해 해당 노드의 정적 값을 추론합니다.
* https://eslint-community.github.io/eslint-utils/api/ast-utils.html#getstaticvalue
*/
const staticValue = getStaticValue(node, context.getScope());
if (staticValue && Number.isInteger(staticValue.value)) {
return true;
}
/**
* 정적 값을 통해 추론되지 않는 프로퍼티의 경우, 타입을 통해 인덱스 타입인지 확인합니다.
* 1. esTreeNodeToTSNodeMap을 통해 ESTree 노드를 타입스크립트 노드로 변환합니다.
* 2. getTypeAtLocation을 통해 해당 노드의 타입을 추론합니다.
* 3. flags를 통해 Number 타입을 확인합니다.
*
* 아래 함수에서 array.length - 1의 타입은 number이므로 인덱스 접근으로 추론됩니다.
*
* function getLast(array: unknown[]) {
* return array[array.length - 1];
* }
*/
const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
const type = typeChecker.getTypeAtLocation(tsNode);
if (type.flags === TypeFlags.Number) {
return true;
}
return false;
}
return {
MemberExpression(node) {
//...isIndexAccess 함수는 두 가지 가정을 통해 인덱스 접근을 판단해요.
1. 해당 컨텍스트에서 추론되는 프로퍼티 노드의 값이 number에 해당할 경우 인덱스 접근일 것이다.
const STATIC_INDEXS = { ARRAY: 5 }
const lastIndex = STATIC_INDEXS.ARRAY - 1;
const value = array[lastIndex];getStaticValue를 사용하면, ESLint가 동작하는 컨텍스트 내부의 참조를 최대한 많이 이용하여 해당 프로퍼티의 값을 평가할 수 있어요.
위의 코드를 예시로 들면 lastIndex에 해당하는 정적 값을 참조하여 최종적으로는 4 라는 정적 값을 알 수 있게 됩니다.
2. 프로퍼티 노드의 타입이 number에 해당할 경우 인덱스 접근일 것이다.
function getLast(array: unknown[]) {
return array[array.length - 1];
}추론된 정적 값으로 number임을 알 수 있다면 좋겠지만, 함수의 인자와 같이 정적인 정보로는 파악이 불가능한 경우도 생기기 마련이에요. 이 경우에는 TSNode를 참조하여 number 타입인지 확인이 가능합니다.
마지막으로 이렇게 찾아낸 케이스에 대해 경고를 띄워줘야겠죠? 해당 노드에 에러 메시지 혹은 경고를 노출하기 위해서 context.report 메소드를 사용할 수 있어요.
//...
/** node.property는 브라켓 내부의 노드입니다. (a[b] 에서 b에 해당) */
if (!isIndexAccess(node.property)) {
return;
}
const indexNode = node.property;
context.report({
node: indexNode,
messageId: PREFER_AT
});
}
//...
그런데 한 가지 아쉬운 점이 있네요. ESLint 에러를 통해 코드 작성 시점에 피드백을 받을 수 있어 생산성은 개선되었지만, 에러 해결을 위해서는 해당 코드를 직접 수정해줘야 하는 불편함이 남아있어요.
하지만 우리는 이미 해결 방법을 알고 있습니다.
ESLint를 사용하다 보면 일부 규칙의 경우 --fix flag를 통해 자동으로 코드의 문제를 해결해주는 경험이 있으실거에요. 마찬가지로 저희 규칙에도 자동 수정 기능을 추가하면 어떨까요?
RuleFixer 를 사용하면 이러한 수정 메소드를 쉽게 구현할 수 있습니다. context.report 에 있는 fix 프로퍼티에 아래와 같은 generator 함수를 선언해줄게요.
context.report({
node: indexNode,
messageId: PREFER_AT,
*fix(fixer) {
const bracketTokens = getOpeningAndClosingBracketTokens(context.getSourceCode(), indexNode);
if (!bracketTokens) {
return;
}
const { openingBracketToken, closingBracketToken } = bracketTokens;
/**
* optional chaining이 있는 경우, a?.[0] -> a?.at(0)으로 변환합니다.
* optional chaining이 없는 경우, a[0] -> a.at(0)으로 변환합니다.
*/
const openingBracketReplaceText = node.optional ? 'at(' : '.at(';
// array[0] -> array.at(0]
yield fixer.replaceText(openingBracketToken, openingBracketReplaceText);
// array.at(0] -> array.at(0)
yield fixer.replaceText(closingBracketToken, ')');
},
});
//...
function getOpeningAndClosingBracketTokens(
sourceCode: Readonly<SourceCode>,
indexNode: Expression,
) {
/**
* indexNode의 앞,뒤 브라켓 토큰을 찾습니다.
* 토큰이 하나라도 없는 경우, null을 반환합니다.
*/
const openingBracketToken = sourceCode.getTokenBefore(indexNode, isOpeningBracketToken);
const closingBracketToken = sourceCode.getTokenAfter(indexNode, isClosingBracketToken);
if (!openingBracketToken || !closingBracketToken) {
return null;
}
return { openingBracketToken, closingBracketToken };
}노드 전후의 브라켓([])을 at() 으로 교체해주는 간단한 동작을 통해 팀원분들의 귀찮음을 줄였네요!
IDE의 ESLint autofix on save 옵션을 사용한다면 이제 일일히 at()으로 교체하는 작업을 하지 않아도 될 것 같네요!
테스트 코드를 통한 검증
그런데 이렇게 만든 ESLint 규칙이 정상적으로 동작한다고 장담할 수 있을까요? 놓친 엣지 케이스가 있거나 동작하지 않아야 하는 상황에서도 동작한다면 어떻게 될까요?
리팩토링 과정에서 실수로 기존 동작에 영향을 줄 수 있는 부분을 수정해서 오작동이 생긴다면 어떨까요? 그 과정에서 팀원들의 생산성이 오히려 떨어지면 안 될 것 같아요.
마찬가지로 우리는 이런 경우에 대처하기 위한 굉장히 좋은 수단을 이미 알고 있습니다. 바로 테스트 코드입니다.
@typescript-eslint/utils 패키지에서는 규칙을 테스트하기 위한 테스터를 내장하고 있으며 사용법도 매우 간단해요.
https://eslint.org/docs/latest/contribute/tests
위 문서를 참고해서 테스트 코드를 작성해 보겠습니다.
const VALID_CASES: Code[] = [
`
const a = [1, 2, 3];
a.at(0);
`,
`
const a: unknown[] | undefined = [];
a?.at(0);
`,
];
const INVALID_CASES: [Code, Output][] = [
[
`
const a = [1, 2, 3];
a[0];
`,
`
const a = [1, 2, 3];
a.at(0);
`,
],
[
`
function accessIndex(a: unknown[], index: number) {
return a[index];
};
`,
`
function accessIndex(a: unknown[], index: number) {
return a.at(index);
};
`,
]
//...
];
ruleTester.run('prefer-at', PreferAtRule, {
valid: VALID_CASES,
invalid: INVALID_CASES.map(([code, output]) => ({
code,
output,
errors: [{ messageId: 'PREFER_AT' as const }],
})),
});위와 같이 통과 케이스, 에러 케이스와 fix 후 결과까지 간단하게 테스트 코드를 작성해서 검증할 수 있어요.
사실 글의 흐름상 테스트 코드의 작성을 맨 마지막으로 미뤘지만, 실제 개발 단계에서는 테스트 코드를 먼저 작성하고 추가하면서 테스트 통과를 목적으로 규칙을 구현하는 TDD 방법론을 통해 개발했어요. 구현 요건이 명확하여 테스트 코드 작성이 간편하고, 개발 루프에서 빠른 피드백을 받을 수 있어 개발 과정에서 큰 도움이 되었습니다.
이렇게 추가된 ESLint 규칙을 통해 앞으로는 at() 사용에 대한 코멘트가 PR리뷰 과정에서 등장할 일은 없게 되었습니다.
마치며
생산적인 코드 리뷰를 하기 위해서는, 코드의 정적인 컨벤션에 대해 소통하는 시간을 줄이고 자동화가 가능한 부분들을 최대한 자동화 하는 것이 중요하다고 생각합니다.
코드 리뷰에 많은 시간을 쏟는 저희 크리에이트립 프론트엔드 파트에서는 내부 구성원들의 합의를 통해 이러한 규칙을 만들어 서로의 시간을 아끼고 더욱 양질의 리뷰를 하기 위해 노력하고 있어요.
오늘 소개해드린 ESLint rule 뿐만 아니라 GA 로깅을 위한 이벤트 스펙 자동화, i18n 번역값 검증, PR 생성 자동화 등 여러 생산성을 높일 수 있는 자동화 프로세스를 구현하여 업무의 효율을 높이고 있습니다.
코드 리뷰를 통한 성장과 업무 생산성의 레버리지를 가져올 수 있는 자동화에 관심이 있다면 언제든 크리에이트립의 문을 두드려 주세요!
