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

[jello] Week4 #55

Open
wants to merge 12 commits into
base: jello
Choose a base branch
from
Prev Previous commit
Next Next commit
fix: resolve circular reference with dispatch
- `dispatch` get new state by `invoke`.
- By subscribers, call every re-render methods of component with new state.
hjsong333 committed Jun 14, 2023
commit 7b14ad3e490a0c8e643c6824ae9658e759694176
6 changes: 3 additions & 3 deletions src/components/main/Main.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import GridView from './gridView/GridView';
import MainHeader from './MainHeader';
import style from './Main.module.css';
import { createElement } from '../../utils/domUtils';
import { MainHeader } from './MainHeader';
Copy link
Member

Choose a reason for hiding this comment

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

주가 되는 것을 default로 export하고 부가적인 것들을 그냥 export하면 짐작을 통한 불필요한 연산없이 예를 들어서, 어떤게 component인건지 구분이 바로 되서 저는 개인적으로는 이게 좋은거 같아요.

import MainHeader, { MainHeaderProps } from "./MainHeader";

import { ListView } from './listView/ListView';
import { ArrowButton } from './ArrowButton';
import GridView from './gridView/GridView';
import style from './Main.module.css';

type MainProps = {
gridInfo: GridInfo;
9 changes: 5 additions & 4 deletions src/components/main/MainHeader.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import { invoke } from '../../store';
import { dispatch, subscribe } from '../../dispatch';
Copy link
Member

Choose a reason for hiding this comment

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

Vite 및 TS에 path alias를 설정해두시면 더 명확하고 안정적이게 import 할 수 있을것 같습니다.
Ex: "@store/dispatch", "@utils/domUtils"

import { createElement } from '../../utils/domUtils';
import style from './MainHeader.module.css';

type MainHeaderProps = {
export type MainHeaderProps = {
mainViewerInfo: {
targetMedia: 'total' | 'subscribed';
viewer: 'listView' | 'gridView';
};
};

export default class MainHeader {
export class MainHeader {
public readonly element;
private tabs;
private viewers;
Copy link
Member

Choose a reason for hiding this comment

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

createNavElement라는 함수로 nav용의 UI를 만드는것 같은데 tabs, viewers 이름만봐서 정확히 어떤 nav를 담당하는지 헷갈리는 것 같습니다.

@@ -24,6 +24,7 @@ export default class MainHeader {

this.element.append(tabNav, viewerNav);
this.setEvent();
subscribe(this.updateView.bind(this))
Copy link
Member

Choose a reason for hiding this comment

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

subscribe는 함수를 받는데 이 함수가 기대하는 type이 this.updateView()와 다른거 같은데 어떻게 동작이 되는건가요?

}

private createTabs() {
@@ -110,7 +111,7 @@ export default class MainHeader {
const viewerName = viewer.getAttribute('data-viewer');
const isActiveViewer = viewer.classList.contains(style.active_viewer!);
if (viewerName && !isActiveViewer) {
invoke({
dispatch({
type: 'changeViewer',
payload: {
viewer: viewerName as 'gridView' | 'listView'
31 changes: 31 additions & 0 deletions src/dispatch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { invoke } from './store';

const subscribers: Function[] = [];

export const subscribe = (
subscriber: (state: {
dateInfo: Date;
gridInfo: GridInfo;
subscriptionInfo: string[];
mainViewerInfo: {
targetMedia: 'total' | 'subscribed';
viewer: 'listView' | 'gridView';
};
news: NewsData | null;
fields: FieldData[];
listIndex: number;
arrowInfo: {
left: boolean;
right: boolean;
};
}) => void
) => {
subscribers.push(subscriber);
};

export const dispatch = (action: Action) => {
const state = invoke(action)!;
if (state) {
Copy link
Member

Choose a reason for hiding this comment

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

Non-null assertion 혹은 type guard 중복이 되서 둘 중하나만 해주시면 될것 같습니다.

subscribers.forEach((subscriber) => subscriber(state));
}
};
16 changes: 0 additions & 16 deletions src/main.ts
Original file line number Diff line number Diff line change
@@ -20,19 +20,3 @@ const newsStand = new NewsStand({
});

app.append(newsStand.element);

export const onChangeState = () => {
newsStand.updateView({
dateInfo: state.dateInfo,
gridInfo: state.gridInfo,
subscriptionInfo: state.subscribedMedias,
mainViewerInfo: {
targetMedia: state.targetMedia,
viewer: state.viewer
},
news: state.news,
fields: state.fields,
listIndex: state.listIndex,
arrowInfo: state.arrowInfo
});
};
6 changes: 3 additions & 3 deletions src/store.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { getNewsList, getSubscribedMedias, setSubscribedMedias } from './utils/dataUtils';
import { GRID_PAGE_LIMIT } from './constants';
import { onChangeState } from './main';

const state: {
dateInfo: Date;
@@ -36,7 +35,7 @@ const state: {
};

export const getState = () => {
return state;
return { ...state };
};

export const invoke = (action: Action) => {
@@ -121,7 +120,8 @@ export const invoke = (action: Action) => {
break;
}

onChangeState();
return {...state};
// onChangeState();
};

const increaseGridPage = () => {