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

Enable customizing reactive rule to treat certain custom functions as reactive #112

Closed
1 task done
CreativeTechGuy opened this issue Dec 7, 2023 · 3 comments · Fixed by #113
Closed
1 task done
Assignees
Labels
enhancement New feature or request

Comments

@CreativeTechGuy
Copy link
Contributor

Describe the need

I am making a library for my team which handles data fetching with our custom protocol. The library will take in the arguments to the API call and return a signal, very similar to the native createResource.

Our library's interface looks something like:

function query(arg: Accessor): Accessor;

This works great and we are very careful to use that signal accessor that is passed to the query method only in a reactive scope. The problem is that to a user who is using the solid/reactivity rule, it marks query(mySignal) as an error. We need a way to indicate that our function, despite being custom, is safe to pass a signal to so that the ESLint rule doesn't improperly warn about it. (But it should still warn if the user instead does query(mySignal()) which would break reactivity.

Suggested Solution

This seems to be already discussed in Implementation v2 under the "Non-extensible" problem bullet point.

Possible Alternatives

We can hack around the limitations of the rule by changing the function signature to function query(arg: () => Accessor): Accessor which then the reactivity rule no longer errors. But this is an already known bug and we don't want to design our API surface around relying on a bug in a linter.

The other option is to disable the rule entirely, or for every occurrence of someone using our query method. This is a slippery slope and will likely hide other actual issues.

Additional context

It'd be great if the rule had an option which could be set in the ESLint config similar to jest/expect-expect.

Our full API calls actually look like client.something.something.something.query(mySignal) with any number of arbitrary something's in the middle (because tRPC). So it'd be ideal if the configuration option checked just the function call query(mySignal) ignoring whatever the user-defined namespace is.

  • I would be willing to contribute a PR to implement this feature
@CreativeTechGuy CreativeTechGuy added the enhancement New feature or request label Dec 7, 2023
@joshwilsonvu
Copy link
Collaborator

Thanks for doing some research and filing an issue!

Would it be possible for you to rename query to something that begins with create* or use*? Similar to React, the rule treats calls on those identifiers as custom hooks, and is as permissive with reactivity as it reasonably can be.

Being able to mark custom functions as “reactive” is a common request and I’d love to make it possible. The main trouble is that it’s not just “is reactive” and “is tracking”—we need something close to a type signature to express exactly how expressions can be reactive. Working on that under v2, but haven’t been able to dedicate a lot of time to it.

@CreativeTechGuy
Copy link
Contributor Author

I actually discovered that create* and use* escape hatch while trying to make the change to this rule. I never noticed that in the docs for the rule, and now that I go back I still don't see it. I must just be missing that somewhere.

I guess what I'm asking for is a way to manually add names to that list, in addition to the create* and use* patterns which already exist. I tested out my change and it's about a dozen lines total mostly leveraging those exact same code paths.

I understand that you want a fancier system for v2, but would you be open to a manual, config-driven solution in the meantime?

@CreativeTechGuy
Copy link
Contributor Author

If that is something you are open to, I've created a very small PR which adds that option. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants