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

feat: enhanced type inference for future api #486

Merged
merged 4 commits into from
Jul 26, 2024

Conversation

malangcat
Copy link
Collaborator

@malangcat malangcat commented Jul 25, 2024

Contribution

Known Issues

  • ActivityParam에는 필요하지만 loader에서는 관심이 없는 필드들이 있습니다.
    • 즉, loader의 인자는 대응되는 Activity의 ActivityParam의 부분집합입니다.
    • 현재 push 등의 액션에서 param type을 추론하는 데 loader를 사용하고 있으나, 실제로는 Activity에서 추론되어야 합니다.
export const loader: ActivityLoader<{ articleId: string }> = ({ params }) => {
  ...
}
export interface ArticleParams {
  articleId: string;
  title: string;
}

위와 같이 주어졌을 때, push("Article", {...}) 에서 타입이 { articleId: string, title: string }으로 추론되어야 하나 { articleId: string }으로 추론되고 있습니다.

demo/src/components/ArticleCard.tsx 에서 재현되고 있습니다.

가능한 대안은 다음과 같습니다.

타입 추론을 loader가 아닌 Activity를 통하도록 변경한다.

디자인 의도상 config에서는 Activity를 import하지 않고 있기 때문에, config보다 상위 레이어에서 해결해야 합니다.

  • config에서 Activity의 타입만 가져올 수 있습니다.
    • 이 경우 paramTypes를 따로 선언하는 기존 접근과 거의 동일해집니다.
  • 혹은, flow({ config }) 에서 stack()에서 반환하는 actions 등의 맥락을 추가로 전달받을 수 있습니다.
    • useFlow()는 loader 번들이 아닌 클라이언트 번들에 어차피 포함되기 때문에, 문제가 없을 것으로 예상합니다.

ActivityParam 인터페이스를 Loader, Activity에서 공유하도록 강제한다.

  • 구현을 거의 변경하지 않고 해결이 가능하지만, ActivityParam 선언을 loader에서 하는 것이 불편해질 수 있습니다.

Decision

-> flow({ config })flow<Actions>()로 인터페이스를 변경하고, stack()이 반환하는 action 오브젝트에 loader, Activity 타입 관심사를 보존하는 방식으로 수정합니다.

Copy link

changeset-bot bot commented Jul 25, 2024

⚠️ No Changeset found

Latest commit: 5d4b83d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jul 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
stackflow-docs ✅ Ready (Inspect) Visit Preview Jul 26, 2024 0:38am

@tonyfromundefined
Copy link
Member

tonyfromundefined commented Jul 25, 2024

export const loader: ActivityLoader<typeof MyActivity> = ({ params }) => {
  // ...
}

이건 어떤가요?

Activity를 import하지 않고 있기 때문에, config보다 상위 레이어에서 해결해야 합니다.

이 전제때문에 힘들겠네요 ㅠ

@malangcat
Copy link
Collaborator Author

export const loader: ActivityLoader<typeof MyActivity> = ({ params }) => {
  // ...
}

이건 어떤가요?

Activity를 import하지 않고 있기 때문에, config보다 상위 레이어에서 해결해야 합니다.

이 전제때문에 힘들겠네요 ㅠ

import type이면 괜찮을수도 있긴 한데, #478 (comment) 에서 제안한 것 처럼 useLoaderData의 타입을 loader에서 가져온다고 생각하면 상호간에 import가 걸리는게 좋은 접근이 아닌 신호로 느껴졌어요.

@tonyfromundefined tonyfromundefined merged commit 73a30f9 into future-api Jul 26, 2024
3 of 4 checks passed
@tonyfromundefined tonyfromundefined deleted the future-api-type-infer branch July 26, 2024 12:35
@tonyfromundefined tonyfromundefined restored the future-api-type-infer branch July 26, 2024 13:52
@malangcat malangcat deleted the future-api-type-infer branch July 31, 2024 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants