solidjs-community / eslint-plugin-solid

Solid-specific linting rules for ESLint.
MIT License
216 stars 26 forks source link

reactivity: Reading a signal asynchronously does not raise a warning #16

Closed otonashixav closed 2 years ago

otonashixav commented 2 years ago

Describe the bug Reading a signal asynchronously should cause the reactivity rule to warn, but it does not.

To Reproduce https://playground.solidjs.com/?hash=725584123&version=1.3.13

Expected behavior A warning is expected as with other untracked signal accesses. If possible, only reading after an await should warn, as before then the call is still synchronous.

joshwilsonvu commented 2 years ago

Hi, I know where you're coming from and agree there should be a warning. I'm curious if you already see a warning on the function that says This tracked scope should not be async. Solid's reactivity only tracks synchronously.? If not, what version of eslint-plugin-solid are you using?

Screen Shot 2022-04-13 at 7 49 29 PM

This warning comes up on the function instead of only on signal reads after awaits because detecting whether a read may come after an await is surprisingly hard, and should be built into ESLint. It's easier to disallow async functions where synchronous functions should be, and the rule handles Promise callbacks naturally.

otonashixav commented 2 years ago

Strange, could've sworn it didn't warn yesterday... closing this issue.

otonashixav commented 2 years ago

Nevermind, figured out how I got it yesterday:

  const myAsyncFunction = async () => {
    await Promise.resolve();
    console.log(count());
  };
  createEffect(myAsyncFunction);
joshwilsonvu commented 2 years ago

Oh, okay, thanks for the reproduction. I think I'm going to say that this is out of scope for the rule, unless there's good evidence otherwise. Do you think that using a variable for a createEffect function is an idiomatic use case?

ESLint doesn't work like TypeScript; where TypeScript treats a function the same regardless of if it's declared inline or assigned to a variable, ESLint looks for static patterns like "function call on createEffect where the first argument is an async function declared inline." So for cases like this, which are rare as far as I know, it requires quite a bit more work to detect if a variable refers to an async function.

otonashixav commented 2 years ago

Makes sense. No, it isn't idiomatic in most cases. Will close this again then. Thanks!