refactor(stackflow): Refactoring stackflow #404
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -1,6 +1,6 @@ | |||
| import type { Activity, Stack, StackflowPlugin } from "@stackflow/core"; | |||
|
|
|||
| export type StackflowReactPlugin<T = never> = () => { | |||
| export type StackflowReactPlugin = () => { | |||
There was a problem hiding this comment.
StackflowReactPlugin의 제네릭이 실제로 사용되지 않는 것 같아 제거하였습니다. 🙏 실제 d.ts 파일을 보더라도 다음과 같습니다
/// <reference types="react" />
import type { Activity, Stack, StackflowPlugin } from "@stackflow/core";
export declare type StackflowReactPlugin = () => {
/**
* Determine how to render by using the stack state
*/
render?: (args: {
stack: Stack & {
render: (overrideStack?: Partial<Stack>) => {
activities: Array<Activity & {
key: string;
render: (overrideActivity?: Partial<Activity>) => React.ReactNode;
}>;
};
};
initialContext: any;
}) => React.ReactElement<any, any> | null;
/**
* Wrap `<Stack />` component with your `Provider` or custom elements
*/
wrapStack?: (args: {
stack: Stack & {
render: () => React.ReactNode;
};
initialContext: any;
}) => React.ReactElement<any, any> | null;
/**
* Wrap an activity with your `Provider` or custom elements
*/
wrapActivity?: (args: {
activity: Activity & {
render: () => React.ReactNode;
};
initialContext: any;
}) => React.ReactElement<any, any> | null;
} & ReturnType<StackflowPlugin>;
There was a problem hiding this comment.
comment: #186 에서 이루어진 변경으로 확인되는데, plugin-history-sync의 타입 오류를 해결하기 위한 변경으로 보이네요.
There was a problem hiding this comment.
@malangcat
question: 히스토리 알려주셔서 감사합니다! 실제로 활용되지 않는데 타입 오류가 발생하는게 의아하네요 정말 제네릭과 관련이 있는 걸까요 🤔
There was a problem hiding this comment.
| @@ -11,8 +11,8 @@ function parseActionOptions(options?: { animate?: boolean }) { | |||
| return { skipActiveState: false }; | |||
| } | |||
|
|
|||
| const isNullableAnimateOption = | |||
| options.animate === undefined || options.animate == null; | |||
| const isNullableAnimateOption = options.animate == null; | |||
There was a problem hiding this comment.
아래와 같은 결과가 나오기 때문에 options.animate === undefined 로직은 없어도 될 것 같습니다. 어떻게 생각하시나요? 🙏
undefined == null // trueThere was a problem hiding this comment.
comment: nullish 체크를 위한 double equal은 의견이 종종 갈리는 부분인데, 작성자의 의도가 중요하다고 생각해요. cc. @irrationnelle
note: Rome tools 에서는 인지적 측면에서 null 비교에 한해 double equal을 허용하도록 예외를 두고 있어요.
There was a problem hiding this comment.
@malangcat
comment: 말씀에 대한 부분은 동의합니다 👍 단, 이 부분은 double equal로 nullish 체크하는 로직이 기존에 이미 존재했고, 따라서 options.animate === undefined 은 불 필요하다고 생각했습니다
There was a problem hiding this comment.
const isNullable = foo === undefined || foo == null 는 const isFalsy = !foo 와 구분해서 null 체크가 필요하다고 생각할 때 평소 사용하는 코드였어요.
코드 리뷰에서 지적하셔서 다시 점검해보고 foo == null 만으로도 충분하다는 것을 알게 되어 별다른 의견은 남기지 않았어요.
요약하면, 제 의도는 isFalsy 와 구분하기 위한 것이었으며, 구현 방식의 의도는 부정확한 지식으로 인해 불필요한 코드가 포함된 것으로 봐주세요.
| @@ -10,7 +10,7 @@ export function useMemoDeep<T>(next: T) { | |||
| if (!isEqual) { | |||
| previousRef.current = next; | |||
| } | |||
| }); | |||
| }, []); | |||
There was a problem hiding this comment.
혹시 의존성 배열 없는게 의도된 부분일까요? useEffect의 의존성 배열을 추가하였습니다 🙏
There was a problem hiding this comment.
이 부분에 빈 의존성 배열을 추가하게 되면 next값이 변해도 previousRef.current가 업데이트 되지 않고 항상 초기값으로 유지되어 next값이 변할때마다 해당 hook을 사용한 컴포넌트에 매번 리렌더링을 발생시키고 그에 따라 성능 하락을 야기할 수 있어요. 해당 수정 사항은 revert 하시거나 의존성 배열을 [next]로 수정해주세요.
There was a problem hiding this comment.
의존성 배열에 [next] 를 추가하는 것이 좀 더 명시적으로 코드의 의도가 드러날 수 있겠네요.
There was a problem hiding this comment.
@irrationnelle @orionmiz
의존성 배열 [next]를 추가하는 것으로 재 수정하였습니다 리뷰 감사합니다 😄
| @@ -11,8 +11,8 @@ function parseActionOptions(options?: { animate?: boolean }) { | |||
| return { skipActiveState: false }; | |||
| } | |||
|
|
|||
| const isNullableAnimateOption = | |||
| options.animate === undefined || options.animate == null; | |||
| const isNullableAnimateOption = options.animate == null; | |||
There was a problem hiding this comment.
comment: nullish 체크를 위한 double equal은 의견이 종종 갈리는 부분인데, 작성자의 의도가 중요하다고 생각해요. cc. @irrationnelle
note: Rome tools 에서는 인지적 측면에서 null 비교에 한해 double equal을 허용하도록 예외를 두고 있어요.
| @@ -1,6 +1,6 @@ | |||
| import type { Activity, Stack, StackflowPlugin } from "@stackflow/core"; | |||
|
|
|||
| export type StackflowReactPlugin<T = never> = () => { | |||
| export type StackflowReactPlugin = () => { | |||
There was a problem hiding this comment.
comment: #186 에서 이루어진 변경으로 확인되는데, plugin-history-sync의 타입 오류를 해결하기 위한 변경으로 보이네요.
| @@ -10,7 +10,7 @@ export function useMemoDeep<T>(next: T) { | |||
| if (!isEqual) { | |||
| previousRef.current = next; | |||
| } | |||
| }); | |||
| }, [next]); | |||
There was a problem hiding this comment.
question: [isEqual, next]이 아니어도 괜찮은지 헷갈리네요😵💫
There was a problem hiding this comment.
@malangcat
comment: isEqual의 compare에서 next를 인자로 활용하고 있기 때문에 의존성 배열로 [next] 만 넣어도 기능상으로는 문제는 없을 것 같다는 생각입니다! 물론[next, isEqual]도 상관없을 것 같습니다 👍
ssi02014
changed the title
| type StackflowPluginsEntry<T extends BaseActivities> = | ||
| | StackflowReactPlugin<T> | ||
| | StackflowReactPlugin | ||
| | StackflowPluginsEntry<T>[]; |
There was a problem hiding this comment.
suggestion: #404 (comment) 에서 제시된 내용에 따라, StackflowPluginsEntry의 Generic도 제거해도 무방해 보이는데, 적용해보면 더 많은 코드를 삭제할 수 있겠네요.
There was a problem hiding this comment.
@malangcat
suggestion: 확인해주셔서 감사합니다! StackflowPluginsEntry는 좀 더 엮인 부분도 있고, 다음 개선사항으로 남겨놓고자 합니다 괜찮을까요? 🙏 디테일한 리뷰에 감사드립니다
좋은 라이브러리를 공유해주셔서 감사합니다. 🙇♂️
저는 코드 일부를 리팩토링 작업을 진행했습니다.
적절한 Pull Request 인지 검토해주시면 감사드립니다!