Skip to content

feat: Database 에서 retrieve 된 ROW를 Required<> 처리한다.#21

Draft
elegantcoder wants to merge 1 commit intomainfrom
feat/find-required
Draft

feat: Database 에서 retrieve 된 ROW를 Required<> 처리한다.#21
elegantcoder wants to merge 1 commit intomainfrom
feat/find-required

Conversation

@elegantcoder
Copy link
Copy Markdown
Contributor

@elegantcoder elegantcoder commented Jun 29, 2022

PR 의 종류는 어떤 것인가요?

  • 새로운 기능

수정이 필요하게된 이유가 무엇인가요? (Jira 이슈가 있다면 링크를 연결해주세요)

Insert 전의 프로퍼티는 optional 일 수 있으나, Create -> Retrieve 된 데이터의 프로퍼티들은 undefined 일 수 없다.

다시말해
Insert 전의 프로퍼티는 undefined / null / not null 중 하나일 수 있지만, Retrieve 후 의 프로퍼티들은 null / not null 만 가능하다.

무엇을 어떻게 변경했나요?

데이터베이스를 거쳐 반환되는 ROW들에 대해 Required<ROW> 처리

코드 변경을 이해하기 위한 배경지식이 필요하다면 설명 해주세요.

구현측에서 nullable 필드를 | null 로 유니언이 필요할 수 있습니다.

디펜던시 변경이 있나요?

어떻게 테스트 하셨나요?

코드의 실행결과를 볼 수 있는 로그나 이미지가 있다면 첨부해주세요.

@github-actions github-actions bot added the enhancement New feature or request label Jun 29, 2022
@elegantcoder elegantcoder requested review from a team, iolo and rageatm June 29, 2022 06:05
@elegantcoder elegantcoder self-assigned this Jun 29, 2022
@elegantcoder elegantcoder changed the title feat: Database 에서 retrieve 후의 ROW는 Required 여야 한다. feat: Database 에서 retrieve 된 다음의 ROW를 Required 처리한다. Jun 29, 2022
@elegantcoder elegantcoder changed the title feat: Database 에서 retrieve 된 다음의 ROW를 Required 처리한다. feat: Database 에서 retrieve 된 다음의 ROW를 Required<> 처리한다. Jun 29, 2022
@elegantcoder elegantcoder changed the title feat: Database 에서 retrieve 된 다음의 ROW를 Required<> 처리한다. feat: Database 에서 retrieve 된 ROW를 Required<> 처리한다. Jun 29, 2022
Copy link
Copy Markdown
Contributor

@iolo iolo left a comment

Choose a reason for hiding this comment

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

RequiredPartial이든 하나만 합시다.

그래서 ROW 타입은 어떻게 선언해야 합니까?
모두 ?를 붙이고 Required를 쓰는 건가요?
모두 ?붙이지 말고 Partial을 쓰는 건가요??

@elegantcoder
Copy link
Copy Markdown
Contributor Author

RequiredPartial이든 하나만 합시다.

그래서 ROW 타입은 어떻게 선언해야 합니까?

모두 ?를 붙이고 Required를 쓰는 건가요?

모두 ?붙이지 말고 Partial을 쓰는 건가요??

전자로 통일되고 있습니다.
타입정의시에 Insert 에 반드시 필요한것은 아닌 필드들에 대해 ?을 적어주고있어요.

@iolo
Copy link
Copy Markdown
Contributor

iolo commented Jun 29, 2022

RequiredPartial이든 하나만 합시다.
그래서 ROW 타입은 어떻게 선언해야 합니까?
모두 ?를 붙이고 Required를 쓰는 건가요?
모두 ?붙이지 말고 Partial을 쓰는 건가요??

전자로 통일되고 있습니다. 타입정의시에 Insert 에 반드시 필요한것은 아닌 필드들에 대해 ?을 적어주고있어요.

좀 멀리가면 이렇게도 할 순 있겠네요... 점점 더 멀어져간다...

interface CrudOperations<ID, ROW, INSERT_ROW=Required<ROW>, UPDATE_ROW=Partial<ROW>> {
  insert(data: INSERT_ROW)
  update(id:ID, data: UPDATE_ROW)
}


export interface SelectOperations<ID extends IdType, ROW extends RowType> {
select(filter?: CrudFilter<ID, ROW>, sorts?: Array<Sort>, relations?: Array<Relation>): Promise<Array<ROW>>;
select(filter?: CrudFilter<ID, ROW>, sorts?: Array<Sort>, relations?: Array<Relation>): Promise<Array<Required<ROW>>>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

현재로썬 filter.projection을 사용해서 특정 컬럼들만 select하게되면 Required가 성립하지 않습니다.

Copy link
Copy Markdown
Contributor

@iolo iolo left a comment

Choose a reason for hiding this comment

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

typescript type/interface와 partial/required만으로 RDBMS table을 온전히 표현하는 것은 불가능합니다(not null인데 optional? id는 optional? db의 기본값은?)
호환성을 깨면서까지 required를 도입할 이유가 없다고 생각됩니다.

@iolo iolo reopened this Jun 30, 2022
Copy link
Copy Markdown
Contributor

@iolo iolo left a comment

Choose a reason for hiding this comment

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

I will abstain

@rageatm
Copy link
Copy Markdown

rageatm commented Jun 30, 2022

null과 undefined가 서로 사맛디 아니하지만 undefined | null과 null을 구분해야 하는 것인지 잘 모르겠습니다.

@elegantcoder
Copy link
Copy Markdown
Contributor Author

우선 이 코드와 연관된 interface 들이 변경되어야 하기 때문에 바로 Merge 하기는 애매한 상태입니다.
따라서 Draft 상태로 전환해놓겠습니다.

@elegantcoder elegantcoder marked this pull request as draft July 5, 2022 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants