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: Support for asynchronous getLoadContext #22

Merged
merged 2 commits into from
Nov 17, 2024

Conversation

ogadra
Copy link
Contributor

@ogadra ogadra commented Nov 15, 2024

Related #21

Support for asynchronous getLoadContext.
The getLoadContext function is already awaited in src/middleware.ts at line 21.
Therefore, we only need to support the type that returns Promise<AppLoadContext>.

※ 英語に自信がないので、日本語でも説明します。
非同期の getLoadContext のサポート。
getLoadContext 関数はすでに src/middleware.tsの21行目でawaitされています。
そのため、GetLoadContext 型が Promise<AppLoadContext> を返せるようにすればよいです。

@ogadra ogadra changed the title Support for asynchronous getLoadContext feat: Support for asynchronous getLoadContext Nov 15, 2024
@yusukebe
Copy link
Owner

Hi @ogadra Thank you for the PR!

@predaytor Will this solve your problem? Can you review this?

@predaytor
Copy link

predaytor commented Nov 16, 2024

@yusukebe yeah! except we also need to check the promise in dev script at src/dev.ts:

app.all('*', async (c) => {
  // ...

  // const remixContext = getLoadContext(args)
  // return handler(c.req.raw, remixContext)

  const remixContext = getLoadContext(args)

  return handler(c.req.raw, remixContext instanceof Promise ? await remixContext : remixContext)
})

@yusukebe
Copy link
Owner

@ogadra @predaytor

I've updated to check if Promise or not and added the type defaultGetLoadContext: 37cda75

Can you review this?

@ogadra
Copy link
Contributor Author

ogadra commented Nov 17, 2024

@yusukebe Thanks for support! Looks good to me!

@yusukebe
Copy link
Owner

Okay! Merging.

@yusukebe yusukebe merged commit b0e39e2 into yusukebe:main Nov 17, 2024
2 checks passed
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.

3 participants