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

[🐞] Custom hook failing when hook function is named starting with use.* #6139

Open
avfirsov opened this issue Apr 20, 2024 · 9 comments
Open
Labels
STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working

Comments

@avfirsov
Copy link

Which component is affected?

Qwik Optimizer (rust)

Describe the bug

I am using custom hook createToggle to create a toggler logic for button:

export type Toggle<State> = {
  toggle: QRL<() => Promise<State>>;
  setState: QRL<(newState: State) => Promise<State>>;
  readonly currentState: Promise<State>;
};

export const createToggle = <State>(states: State[]): Toggle<State> => {
  const state = useSignal(0);
  const getCurrentState = $((): State => states[state.value]);

  return {
    toggle: $((): Promise<State> => {
      console.log("=>(createToggle.ts:16) state.value", state.value);
      state.value = (state.value + 1) % states.length;
      console.log("=>(createToggle.ts:16) state.value", state.value);
      return getCurrentState();
    }),
    setState: $((newState: State): Promise<State> => {
      state.value =
        states.indexOf(newState) === -1
          ? state.value
          : states.indexOf(newState);
      return getCurrentState();
    }),
    get currentState(): Promise<State> {
      return getCurrentState();
    },
  };
};

Usage:

export default component$(() => {
  const toggle = createToggle(["Foo", "Bar"]);

  return (
    <>
      <span>{toggle.currentState}</span>
      <Btn onClick$={() => toggle.toggle()} />
    </>
  );
});

The problem is, if I use the name useToggle instead of createToggle for the hook variable, it doesn't work. It seems like component crashes.

Repo for local reproduction: https://github.com/avfirsov/qwik-use-foo-issue-reproduction/tree/main (also deployed on Vercel, see link)

Reproduction

https://qwik-use-foo-issue-reproduction.vercel.app/

Steps to reproduce

No response

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (32) x64 AMD Ryzen 9 7945HX with Radeon Graphics
    Memory: 1.47 GB / 15.69 GB
  Binaries:
    Node: 18.17.1 - C:\Program Files\nodejs\node.EXE
    npm: 9.6.7 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Chromium (123.0.2420.97)
    Internet Explorer: 11.0.22621.1
  npmPackages:
    @builder.io/qwik: ^1.5.2 => 1.5.2
    @builder.io/qwik-city: ^1.5.2 => 1.5.2
    undici: * => 6.13.0
    vite: ^5.1.6 => 5.2.10

Additional Information

No response

@avfirsov avfirsov added STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working labels Apr 20, 2024
@wmertens
Copy link
Member

On my phone, but I see that you define a getter, that doesn't work, all state objects must be plain objects and all functions must be qrls.

Also, you can use https://qwik.dev/playground for testing this, that way you can easily inspect optimizer output and use different versions

@avfirsov
Copy link
Author

Yeah, I get your point and all the requirements you mentioned are met. But the point is that getter doesn’t work if the variable is called in a specific way. It seems like the name of a variable plays important role whether getter works or not. Seems like an issue🤷‍♂️

@wmertens
Copy link
Member

wmertens commented Apr 21, 2024

And here's a working example of the state machine, the getter was interfering with the reactivity. Note the little trick of embedding it into a helper object so the qrls can reference "self". This state machine object is self-contained.

https://qwik.dev/playground/#f=7VfNSsNAEH6VJSAkGKM96CE0ETyIB6Eo3koOgmkRWoOxnkre3W9m9mc2NArFi2AuYed3d%2BabmV0NmkudqZD%2F3Dw83udhBucGJO77i9422dwIaxpgPh6c8aduvd60c266NeJCEdkxrSRnc4lT7MNKA2uG2hSvtLSwwcSU0kzMArvk3w%2BW%2BX4CUbltyAhjo0QHeFh02VDo4gzTTYJPgOOIwbxOR1pZOT65gMHeg3gEVSHQ6QVy5NnbdttF3AAmYzBT39r2BZN91wFbmAEtDVBM6ZVk5L3ffLCk84XJUCLNJDHe1UAZGdC06YbASiyOooY1SZYQ6ZA2MrkluizqriGfOIZHrca%2BgvVI2F3dkqpOfXSB1HJ2lYl6gR782SbWmKwQMmdEUan29PLUzDJzYjdRyIQMir%2Fi3VZ4OFzhUJt6qSFzcQuIdpFTirLN5ZgiLhtvIwAfNg7B%2FshkRHH0VFcTBV8gFivvEa6qypzNlKQx19pKxCkn7RwXSwZuBGiqoQM2QB0y4lkOFZjdn6rub%2BehhFEwL7UpjtNlctt1gEZy89wnjZTxRMun50%2B9FxvqWMP8nDm%2BufOrKzw29rILqyc%2FaNV3r%2F6Fppv9%2F4D%2BowP6Cw

import { component$, QRL, useSignal, $, ValueOrPromise, Signal } from '@builder.io/qwik'

export type Toggle<State> = {
  toggle: QRL<() => ValueOrPromise<State>>;
  getState: QRL<() => State>
  setState: QRL<(newState: State) => ValueOrPromise<State>>;
  state: Signal<number>
  states: State[]
};

export const useToggle = <State,>(states: State[]): Toggle<State> => {
  const state = useSignal(0);

  const memo = useSignal(() => {
    // needed to reference self from qrls
    const store: { self: Toggle<State> } = {} as any
    store.self = {
      state, states,
      toggle: $(() => {
        const { state, states } = store.self
        console.log("=>(useToggle.ts:16) state.value", state.value);
        state.value = (state.value + 1) % states.length;
        console.log("=>(useToggle.ts:16) state.value", state.value);
        return store.self.getState();
      }),
      getState: $(() => store.self.states[store.self.state.value]),
      setState: $((newState: State) => {
        const { state, states } = store.self
        state.value =
          states.indexOf(newState) === -1
            ? state.value
            : states.indexOf(newState);
        return store.self.getState();
      }),
    } as Toggle<State>

    return store.self
  })

  return memo.value
};

export default component$(() => {
  const toggle = useToggle(["Foo", "Bar"]);

  return (
    <>
      <span>{toggle.getState()}</span>
      <button onClick$={() => toggle.toggle()}>Hi</button>
    </>
  )
});

@avfirsov
Copy link
Author

avfirsov commented Apr 21, 2024

Hmm, I put your code in the playground and renamed it to useToggle and it just works 🤔

https://qwik.dev/playground/#f=7VfNSsQwEH6VUBRSjNVePJQ2goJ4EETxVvYguC7CsgVxT6Xv7vwlmRS7IngR7CUkmUzmm%2FkyM81Ik%2FCMKfzOPDzeuVSCnTlapFFETXF9Gjab7bql1OoBPeL%2BoLUGdbbsDcnTIgZUMlhnaCZikM1lSsPSKXgPL8NuCylpDxlwF1TkkgQ6jw32AGQVmMhSzlvqU4AsNO9XZTNHw2GUDoaKR5d8ZM%2FBu2EXcuq1sgjk8EmJalTEd%2FU0VJBW9usVHE8kwYuS5%2FhwjiqYg5%2FueorO2wgPgtzUFyVfx%2FcUTs%2FIZvzUGpJWT09MXZpjMbni0hKO%2FcLNgnjmMiv7U%2BloTPwAZ8zZccg3GbD4UgQMFcL716gQeNZ15rSOcsZcag1qvVnU8TNg2G9p9n4R6QDlG4WoD5iuyH4wsTNTmWJMZI6c7YubYYBgFVfP70VOy3nuwj7ej6yj0iim9oz2Yp6iH4jUN49sh5zkwZaTv32LPxs6b%2F3Xmj9aaz4B

(It also puts Foo outside the html which is a bug but which I think is already fixed in v2 so that's not relevant here)

Thank you for putting my example into playground. Actually, it perfectly reproduces the bug I described: please pay attention to the text content of <span>{toggle.currentState}</span> with useToggle and createToggle. For your convenience I modified slightly your playground:

https://qwik.dev/playground/#f=7VdLS8NAEP4rS0DY4BpNBJGYRFAQD4Io3kIPgrUIpQGxp5L%2F7ryyOwlNfNBLobksm533fDszq0FzfqkyFfLvzNPzgws92JkJIHm%2FKbMvzWKxnBdUXCvwHz3%2Fon85Ci04HlKphQzAZLDT0E7IoJ7LlpYxLrgRb81qCUVpDTVw1YnoU5Lb%2FezwVWJjwUomdJWlYQUQQ%2Ft6FudDhziXMsZQBylDnOwZhLg7hcJ6q4wCOrxXIhoFsa6algRqy3o%2BA%2FaAFFQUgsfMfcc6c%2FDTo09UVhbnHGKFTOfpRczqWE%2Fk9I5sxk%2F9Q%2BTq7bFJY3MkJifcXzq2HWgWjwchs3Lexo7WABEIxhAgU7HpOeavizhD3fDx3QsEqJWlOUk9nTHXWoL6n4%2FK%2BJtjOHRpAG%2FJdOfKDwJRHoAd8c4o9MmAjGrQX5nf1X%2BWwiAciLB1dNc0kNHo5vUzQuz2yTO%2BGmPE4yURnwfVhoUkOi5tcUpnvvzRuySM4xu2Wzh5sXFb3X%2F4N8w2Hdn%2FlWQTWqToHhrlnjbKbw

@avfirsov
Copy link
Author

avfirsov commented Apr 21, 2024

const memo = useSignal(() => {
    // needed to reference self from qrls
    const store: { self: Toggle<State> } = {} as any
    store.self = {
      state, states,
      toggle: $(() => {
        const { state, states } = store.self
        console.log("=>(useToggle.ts:16) state.value", state.value);
        state.value = (state.value + 1) % states.length;
        console.log("=>(useToggle.ts:16) state.value", state.value);
        return store.self.getState();
      }),
      getState: $(() => store.self.states[store.self.state.value]),
      setState: $((newState: State) => {
        const { state, states } = store.self
        state.value =
          states.indexOf(newState) === -1
            ? state.value
            : states.indexOf(newState);
        return store.self.getState();
      }),
    } as Toggle<State>

    return store.self
  })

  return memo.value

Yeah, I got your idea about self-containing state maching, that's an interesting solution, thank you! But what's the need of storing state machine in signal? What's the point of memo variable? Why not just useToggle shoud return store.self? It seems to also be a working solution

@wmertens
Copy link
Member

aha, it's a bug, but not what you think because the generated code looks the same, and when you switch the order of the lines it's still happening.

We'll need to check if this still happens in v2.

As for the memo, that way the object only gets created once and you can put it in a context

@wmertens
Copy link
Member

oh and actually, since you're using a getter I don't think we should dig much further into this issue, since those aren't supported. They look like plain values to the serializer and therefore reactivity is broken.

So I propose we close this issue.

@avfirsov
Copy link
Author

avfirsov commented May 7, 2024

oh and actually, since you're using a getter I don't think we should dig much further into this issue, since those aren't supported. They look like plain values to the serializer and therefore reactivity is broken.

But why then my approach is still working when using your solution? Could you please explain what you mean by reactivity? Isn't getter in your example reactive?

As someone from the Vue world, I find getters like this highly convenient for code reusability and composability. It would be really upsetting if they would not be supported by Qwik

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants