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(core): useConstant, createSignal, createComputed$ #6319

Merged
merged 3 commits into from
May 27, 2024
Merged

Conversation

wmertens
Copy link
Member

@wmertens wmertens commented May 15, 2024

This adds the following APIs:

  • useConstant(value): stores a fixed value for the lifetime of the component
  • createSignal(value): same as useSignal but can be called a variable amount of times and always returns a new Signal
  • createComputed$(cb): same as createSignal but for computed signals

Note that the create* API calls still need to run within the component context. This is only available inside the render function and the useConst callback.

Perhaps we should also make the context available inside tasks, as long as we forbid calling hooks (by forbidding useSequentialContext).

Example:

  const signals = useConstant(() => {
    const arr: (Signal<number> | string)[] = [];
    for (let i = 0; i < 10; i++) {
      arr.push(createSignal(0));
      arr.push(", ");
    }
    return arr;
  });
  const doubles = useConstant(() =>
    signals.map((s: Signal<number> | string) =>
      typeof s === "string" ? s : createComputed$(() => s.value * 2),
    ),
  );

TODO

  • make createSignal work at any time. Does it really need the container context?
  • make createComputed$ not need a Task so it's not bound to an element, or else don't add this yet. Ideally, the qrl is called and cached in the getter, and when one of the used signals changes the cache is cleared and subs triggered.

Copy link

netlify bot commented May 15, 2024

Deploy Preview for qwik-insights ready!

Name Link
🔨 Latest commit ad15e34
🔍 Latest deploy log https://app.netlify.com/sites/qwik-insights/deploys/6646b8230eee3b000865244e
😎 Deploy Preview https://deploy-preview-6319--qwik-insights.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

cloudflare-workers-and-pages bot commented May 15, 2024

Deploying qwik-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: ad15e34
Status: ✅  Deploy successful!
Preview URL: https://b43a090e.qwik-8nx.pages.dev
Branch Preview URL: https://createsignal.qwik-8nx.pages.dev

View logs

PatrickJS
PatrickJS previously approved these changes May 15, 2024
thejackshelton
thejackshelton previously approved these changes May 16, 2024
@PatrickJS PatrickJS changed the title feat(core): useConst, createSignal, createComputed$ feat(core): useConstant, createSignal, createComputed$ May 17, 2024
@wmertens wmertens requested review from mhevery and removed request for maiieul May 17, 2024 05:36
@wmertens wmertens marked this pull request as draft May 18, 2024 06:59
Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Member

@gioboa gioboa left a comment

Choose a reason for hiding this comment

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

You rock 🚀

@PatrickJS
Copy link
Member

@wmertens can we merge this and have the TODO in another PR?

@wmertens wmertens dismissed stale reviews from thejackshelton and PatrickJS via ad15e34 May 25, 2024 14:56
@wmertens wmertens marked this pull request as ready for review May 27, 2024 16:32
@wmertens wmertens requested review from a team as code owners May 27, 2024 16:32
@wmertens wmertens merged commit f51259b into main May 27, 2024
33 checks passed
@wmertens wmertens deleted the createSignal branch May 27, 2024 16:34
@wmertens
Copy link
Member Author

wmertens commented May 27, 2024

I merged because I think the API will remain the same even though we'll be able to call it in more places.

Here's how to use it dynamically within the component render function. This could also be part of a hook function that forces rerenders.

import { component$, useConstant, createSignal, useSignal, useTask$ } from '@builder.io/qwik';

export default component$(() => {
  const rerender = useSignal(0)
  // force rerender by referencing the signal in the render body
  console.log('render', rerender.value)

  const sigs = useConstant([])

  sigs.push(createSignal(0))

  useTask$(({track}) => {
    track(rerender)
    for (const s of sigs) s.value++
  })

  return (
    <main>
      <div>Counts: {sigs.map(s => <span>{s.value}, </span>)}</div>
      <p>
        <button onClick$={() => rerender.value++}>Click</button>
      </p>
    </main>
  );
});

@barel-mishal
Copy link
Contributor

The API looks interesting, but I see a few issues. Firstly, the main issue for me is that it complicates the API and makes it a bit confusing to understand what I should work with. Should I use useConstant or useSignal?

The use of createComputed inside the function is new to me. Can useSignal also accomplish this? Why is it placed inside the function? It looks a bit confusing in my opinion.

By the way, in the last example, rerender.value is used. I didn't know that Qwik does this rerender if we call the signal in the render. What are the benefits of this?

Additional Confusing Points:

  1. Dynamic Usage of useConstant:

    • The dynamic usage of useConstant within the render function is unclear. Why are we initializing the sigs array with useConstant? Would useSignal or a regular array be more appropriate here?
  2. Forcing Rerenders with Signals:

    • The example forces rerenders by referencing rerender.value in the render body. This approach seems implicit. What other scenarios would benefit from such a method, and are there alternative, clearer ways to trigger rerenders?
  3. Tracking Signals with useTask$:

    • The use of track(rerender) inside useTask$ is confusing. Why is tracking necessary here, and what exactly does it achieve? Could you elaborate on the mechanics and benefits of tracking signals in this context?

@shairez
Copy link
Contributor

shairez commented Jun 11, 2024

@wmertens isn't it marked as deprecated?

@wmertens
Copy link
Member Author

@shairez indeed, but we did invite feedback for this tech preview.

@barel-mishal useConstant can/should be used instead of useSignal if the only reason you use it is to store a fixed value for the lifetime of the component, and you don't need reactivity. For example, a unique id for the current component. Note that useSignal() is the same as useConstant(() => createSignal().

createComputed$ is the counterpart of createSignal. Both should be used when you need to create signals dynamically. useSignal and useComputed$ can only create signals statically.

rerender.value is read in the render function so that when rerender changes, it will cause the render function to rerun.

The reason for that is that createSignal only works inside the render function and inside the useConstant callback, nowhere else. The reason for that is that it needs to know the current container and that's only available there. So it needs to rerender in order to dynamically create signals.
In #6443 we're trying to make it not need the container and then it works from anywhere.

  1. No, you can't create a variable number of signals with useSignal due to the rules of hooks
  2. Indeed it's implicit, and there's no other ways of doing this. Qwik tries to avoid rerenders as much as possible.
  3. It needs to track it so that the task runs again when rerender changes.

Hope this clears things up?

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.

7 participants