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

solid/reactivity for observable #89

Closed
1 task done
voliva opened this issue Mar 25, 2023 · 2 comments · Fixed by #90
Closed
1 task done

solid/reactivity for observable #89

voliva opened this issue Mar 25, 2023 · 2 comments · Fixed by #90
Assignees
Labels
bug Something isn't working

Comments

@voliva
Copy link
Contributor

voliva commented Mar 25, 2023

Describe the bug
inline functions passed into observable from solid-js are maked as invalid as per solid/reactivity

To Reproduce

import { observable } from 'solid-js';

const Component = props => {
  const count$ = observable(() => props.count);

  ...
}

In this case the function is marked as invalid with This function should be passed to a tracked scope (like createEffect) or an event handler because it contains reactivity, or else changes will be ignored., but it is one of the intended ways of using observable if I'm not wrong (please correct me if it is wrong, I'm still new at SolidJS!)

  • I would be willing to contribute a PR to fix this issue

I saw the upcoming refactor at #87, along with the note on the docs which say that maintaining the current version is hard to do. I have some experience writing eslint plugins so I'd like to give it a try, but if you think it's not worth it I might just wait until the refactor is done.

@voliva voliva added the bug Something isn't working label Mar 25, 2023
@joshwilsonvu
Copy link
Collaborator

Hi, thanks for offering to help! You’re right that there’s currently not support for observable and adding support would be great.

Sorry if those notes give the impression that the current rule is totally unmaintainable—it’s not hard to make changes like this so now is a good time. It’s talking more about complex control flow analysis and handling things like curried functions.

It looks like observable is to be treated by the rule in the same way as createEffect, so search in the rule where it marks the createEffect argument as a tracked scope and just add observable there. Should be a small change.

Thanks again!

@voliva
Copy link
Contributor Author

voliva commented Mar 25, 2023

Yes, that was trivial :D

The notes did sound as it was hard to change anything so I got scared about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants