Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

[✨] support qwik functions(use*) after async callsites(await...) #99

Closed
revintec opened this issue Jan 31, 2023 · 8 comments
Closed
Labels
[STAGE-2] incomplete implementation Remove this label when implementation is complete [STAGE-2] not fully covered by tests yet Remove this label when tests are verified to cover the implementation [STAGE-2] unresolved discussions left Remove this label when all critical discussions are resolved on the issue [STAGE-3] docs changes not added yet Remove this label when the necessary documentation for the feature / change is added [STAGE-3] missing 2 reviews for RFC PRs Remove this label when at least 2 core team members reviewed and approved the RFC implementation

Comments

@revintec
Copy link

revintec commented Jan 31, 2023

in version 0.18.1, latest version as of writing, we can't use qwik functions(use*) after await
this limitation is here to because when qwik functions are called, a proper executionContext must be recoverable

but it's an implementation detail and it makes qwik harder to use, requires more boilerplate code(useSignal<...>(...), useTask$(async...), if(!signal.value)return, etc.) when async callsites are involved

I propose a way to solve this, in 2 steps

  1. implements const restoreEnv=useCallsite() or const restoreEnv=useAsync() by
    function useCallsite(){const ctxt=tryGetInvokeContext();return()=>void(_context=ctxt);}
    I actually appended export const USE_CONTEXT=()=>{const ctxt=_context;return()=>_context=ctxt} to core.mjs for testing, and currently found no major problems. there're however some edge cases and warnings

then users can use it like this:

  component$(async(...)=>{
    ...
    const restoreEnv=USE_CONTEXT()
    await ...
    restoreEnv()
    ...
    useSignal(...)
    ...
  }
  1. in the future, the qwik compiler should be able to detect await callsites, and add restoreEnv(...) automatically
    but this is tricky so we can implement 1 first, which should be easier to implement and reason about

original POST follows

Is your feature request related to a problem?

currently code like export default component$(async()=>...) would compile and work fine
notice the async there. but vscode is reporting syntax error

Argument of type '() => Promise<JSXNode<any> | null>' is not assignable to parameter of type 'OnRenderFn<{}>'.
  Type 'Promise<JSXNode<any> | null>' is missing the following properties from type 'JSXNode<any>': type, props, key
ts(2345)

Describe the solution you'd like

can we modify https://github.com/BuilderIO/qwik/blob/70880b71995361202ae0b4725454cb2bd008ab2d/packages/qwik/src/core/component/component.public.ts to update the component$ signature?

from export type OnRenderFn<PROPS> = (props: PROPS) => JSXNode<any> | null
to export type OnRenderFn<PROPS> = (props: PROPS) => ValueOrPromise<JSXNode<any> | null>

it's a small change but I don't know if it's acceptable/compatible/safe

Describe alternatives you've considered

export default component$(()=><>{(async()=>...)()}</>)

but that's ugly and verbose

Additional context

No response

@DustinJSilk
Copy link

You shouldnt be putting async code directly in a component function.
Try place your async tasks in a useTask$ hook instead, and update a store/signal with the new state.

@revintec
Copy link
Author

@DustinJSilk that sure is an alternative solution, but it still is more verbose
why not using async code? is it because of the latency to FCP or the inability to display loading status?

@revintec
Copy link
Author

BTW accessing signal(useSignal()) value(signal.value) after await seems not working

@shairez
Copy link
Contributor

shairez commented Feb 16, 2023

Hi @revintec !

This is not a simple change
We used to have async for component$ but we removed it

According to @manucorporat , adding async await ability to component functions messes up the execution context.

@manucorporat
Copy link

BTW accessing signal(useSignal()) value(signal.value) after await seems not working

right! that's why we dont allow it. this is not a good idea, all this processing should be done inside a useTask!

@revintec
Copy link
Author

revintec commented Feb 19, 2023

thanks for the history, I didn't know that
however I've noticed that useTask$(...) do support async functions, and there're already vscode linters telling people that using qwik functions after await may not gonna work, is it possible to allow users continue using async in component$(...) thus simplifying their code, but if they use any qwik functions we use (the already existing) linter to warn them?

below is an example with only one async/await callsite, image 3(which should be common

use case that allows async:

component$(async()=>{
  const somedata=await(await fetch(...)).json()
  return <>{JSON.stringify(somedata)}</>
})

if not, we have to add more boilerplate code, it's like callback hell all over again:

interface UselessDefinition{dataRef:Signal<...>}
const UselessComponentToIsolateReendering=component$<UselessDefinition>(({dataRef})=>{
  return<>{dataRef.value?JSON.stringify(dataRef.value):"loading maybe? but this text won't show up anyways"}</>
})
component$(()=>{
  const uselessIntermediateVariable=useSignal<...>()
  fetch(...).then((uselessName)=>{
    uselessName.json().then((uselessName2)=>{
        uselessIntermediateVariable.value=uselessName2
    }).catch(...)
  }).catch(...)
  if(!uselessIntermediateVariable.value)return; // this line triggers a error, but it works at runtime
  return<UselessComponentToIsolateReendering dataRef={uselessIntermediateVariable}/>
})

maybe a little exacerbated because of the long variable/type name, but you get the point

if we assume useTask$(...) can handle async functions(and continue to support them
we can replace fetch(...).then((...)=>then...) with useTask$(async()=>await...)

@revintec
Copy link
Author

revintec commented Feb 19, 2023

about the execution context, I understand why they got messed up, and it seems we can restore them like this

component$(({restoreExecutionContext})=>{
  ...
  await ...
  restoreExecutionContext()
  ...
  qwikFunctionCall...
  return ...
})

but I think that's not very user-friendly, so until a better idea come along, let's leave that to another PR :)

I've edited the original post according to this

@revintec revintec changed the title [✨] support async component function in core.d.ts signature [✨] support qwik functions(use*) after async callsite Feb 20, 2023
@revintec revintec changed the title [✨] support qwik functions(use*) after async callsite [WIP] [✨] support qwik functions(use*) after async callsite Feb 20, 2023
@revintec revintec changed the title [WIP] [✨] support qwik functions(use*) after async callsite [WIP] [✨] support qwik functions(use*) after async callsite(await...) Feb 20, 2023
@revintec revintec changed the title [WIP] [✨] support qwik functions(use*) after async callsite(await...) [WIP] [✨] support qwik functions(use*) after async callsites(await...) Feb 20, 2023
@revintec revintec changed the title [WIP] [✨] support qwik functions(use*) after async callsites(await...) [✨] support qwik functions(use*) after async callsites(await...) Feb 20, 2023
@gioboa
Copy link
Member

gioboa commented Oct 14, 2024

We moved this issue to qwik-evolution repo to create a RFC discussion for this.
Here is our Qwik RFC process thanks.

@gioboa gioboa transferred this issue from QwikDev/qwik Oct 14, 2024
@github-actions github-actions bot added [STAGE-2] incomplete implementation Remove this label when implementation is complete [STAGE-2] not fully covered by tests yet Remove this label when tests are verified to cover the implementation [STAGE-2] unresolved discussions left Remove this label when all critical discussions are resolved on the issue [STAGE-3] docs changes not added yet Remove this label when the necessary documentation for the feature / change is added [STAGE-3] missing 2 reviews for RFC PRs Remove this label when at least 2 core team members reviewed and approved the RFC implementation labels Oct 14, 2024
@QwikDev QwikDev locked and limited conversation to collaborators Oct 14, 2024
@gioboa gioboa converted this issue into discussion #175 Oct 14, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
[STAGE-2] incomplete implementation Remove this label when implementation is complete [STAGE-2] not fully covered by tests yet Remove this label when tests are verified to cover the implementation [STAGE-2] unresolved discussions left Remove this label when all critical discussions are resolved on the issue [STAGE-3] docs changes not added yet Remove this label when the necessary documentation for the feature / change is added [STAGE-3] missing 2 reviews for RFC PRs Remove this label when at least 2 core team members reviewed and approved the RFC implementation
Projects
None yet
Development

No branches or pull requests

5 participants