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

[🐞] args of the component$ are not mutable. #6423

Open
genki opened this issue May 29, 2024 · 5 comments
Open

[🐞] args of the component$ are not mutable. #6423

genki opened this issue May 29, 2024 · 5 comments
Labels
COMMUNITY: PR is welcomed We think it's a good feature to have but would love for the community to help with the PR for it COMP: DX Developer Experience related issue COMP: eslint COMP: typescript STATUS-2: waiting for community PR TYPE: enhancement New feature or request

Comments

@genki
Copy link
Contributor

genki commented May 29, 2024

Which component is affected?

Qwik Runtime

Describe the bug

The props of the component$ are given as the args of the QRL, so they should be mutable, but they are declared as const to be immutable through the useLexicalScope. This makes mismatch.
So if I try to change the value of the args, it causes TypeError: Assignment to constant variable. even if there's no error in compile time.

Reproduction

https://stackblitz.com/edit/qwik-starter-c2m93t?file=src%2Froutes%2Findex.tsx

Steps to reproduce

Please see the link above.

System Info

System:
    OS: macOS 14.4.1
    CPU: (8) arm64 Apple M2
    Memory: 93.02 MB / 24.00 GB
    Shell: 3.6.1 - /opt/homebrew/bin/fish
  Binaries:
    Node: 20.12.1 - /opt/homebrew/opt/node@20/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 10.5.0 - /opt/homebrew/opt/node@20/bin/npm
    pnpm: 8.7.5 - ~/Library/pnpm/pnpm
    bun: 1.1.3 - ~/.bun/bin/bun
  Browsers:
    Chrome: 125.0.6422.113
    Safari: 17.4.1
  npmPackages:
    @builder.io/qwik: file:../clone/qwik/packages/qwik/dist => 1.5.3-dev20240507001309 
    @builder.io/qwik-city: file:../clone/qwik/packages/qwik-city/lib => 1.5.3-dev20240507001309 
    typescript: ^5.3.3 => 5.3.3 
    undici: ^5.28.3 => 5.28.4 
    vite: ^5.1.5 => 5.2.11

Additional Information

No response

@genki genki added STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working labels May 29, 2024
@maiieul
Copy link
Contributor

maiieul commented May 31, 2024

I'm not sure, this might be intended behavior to enable lazy-loading/lazy-execution.

May I ask what's your use case? You could use a signal instead:

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

type FooProps = {
  testSig: Signal<number>;
};

const Foo = component$<FooProps>(({ testSig }) => {
  // eslint-disable-next-line qwik/no-use-visible-task
  useVisibleTask$(() => {
    testSig.value = ++testSig.value;
  });
  return (
    <>
      <div>{testSig}</div>
    </>
  );
});

export default component$(() => {
  const testSig = useSignal(2);
  return (
    <>
      <Foo testSig={testSig} />
      {/* I moved all logic to another file in-case you want to quickly delete and prototype something */}
      <div style={{ padding: '1rem' }}>Hello World</div>
    </>
  );
});

@maiieul maiieul added STATUS-2: requires discussion Requires further discussion before moving forward WAITING FOR: team Waiting for one of the core team members to review and reply and removed TYPE: bug Something isn't working STATUS-1: needs triage New issue which needs to be triaged labels May 31, 2024
@genki
Copy link
Contributor Author

genki commented Jun 1, 2024

@maiieul Yes, I understand this is intended.
But I think it is better if warned before the run such as by the tsc or the linter.

@shairez shairez added the TYPE: bug Something isn't working label Jun 1, 2024
@shairez
Copy link
Contributor

shairez commented Jun 1, 2024

Thanks @genki !

Yes I agree that it's a better DX if typescript or eslint could show an error in the code editor.

If anyone has experience with writing eslint rules and want to take a look at one of our existing rules to tackle this, go ahead!
(or if @wmertens knows so some handy TS trick to make it happen, feel free to share 😊 )

So marking it as "PR is welcomed" for now, and we might tackle this after V2

Thanks again!

@shairez shairez added COMP: eslint COMP: DX Developer Experience related issue COMP: typescript COMMUNITY: PR is welcomed We think it's a good feature to have but would love for the community to help with the PR for it STATUS-2: waiting for community PR TYPE: enhancement New feature or request and removed TYPE: bug Something isn't working STATUS-2: requires discussion Requires further discussion before moving forward WAITING FOR: team Waiting for one of the core team members to review and reply labels Jun 1, 2024
@PatrickJS
Copy link
Member

definitely use signals for this but yeah better lint/errors explaining why is a good idea and maybe we can suggest using signals.

what you want to do is either pass a signal or set the initial value of the signal to the prop value depending on what you're trying to do

@genki
Copy link
Contributor Author

genki commented Jun 5, 2024

@PatrickJS
Yes, using the signal can avoid this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMMUNITY: PR is welcomed We think it's a good feature to have but would love for the community to help with the PR for it COMP: DX Developer Experience related issue COMP: eslint COMP: typescript STATUS-2: waiting for community PR TYPE: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants