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

Rule: no-watch #48

Closed
binjospookie opened this issue Sep 28, 2021 · 10 comments · Fixed by #51
Closed

Rule: no-watch #48

binjospookie opened this issue Sep 28, 2021 · 10 comments · Fixed by #51
Labels
enhancement New feature or request

Comments

@binjospookie
Copy link
Sponsor Contributor

Watch have last computation priority.

Frequent use of the watch can lead to races that are very difficult to debug. Often, the use of the watch can be avoided (by using target, for example).

@igorkamyshev
Copy link
Member

igorkamyshev commented Sep 29, 2021

But effects has the same computation priority 🤔

I agree, we can forbid .watch because it leads to imperative code, but I cannot find the connection with computation priority.

@igorkamyshev igorkamyshev added the enhancement New feature or request label Sep 29, 2021
@binjospookie
Copy link
Sponsor Contributor Author

but I cannot find the connection with computation priority

forward({
  from: fooEvt,
  to: barEvt
});
fooEvt.watch(barEvt);

The second option can lead to an "unexpected" calculation order.

@binjospookie
Copy link
Sponsor Contributor Author

binjospookie commented Sep 29, 2021

If u want I can try to implement this rule. Both my issues are not "do it for us" :)

I just need an approve of the idea so I don't waste my time.

@igorkamyshev
Copy link
Member

So, I agree, it's a good rule. Let's do it. I think we ought to mark this rule as warn.

@binjospookie
Copy link
Sponsor Contributor Author

Maybe u can give me some recommendations how to write new rule? I wrote rules many-many years ago :c

@igorkamyshev
Copy link
Member

Oh, I don't think so 🥲 my only advice: try to write as many tests as you can. ESLint plug-in documentation is a really poor.

@binjospookie
Copy link
Sponsor Contributor Author

binjospookie commented Sep 30, 2021

Need a little help. What we should to do with the JS way branch? .watch can be called in a file that does not declare a watchable unit. We can't rely on unit naming, because there is no naming convention for events.

@igorkamyshev
Copy link
Member

igorkamyshev commented Oct 1, 2021

Such a good question. I don't have an answer for you.

Maybe, we ought to introduce #32 before?

But, it isn't resolving a problem with something like sample(...).watch(...) and so on. So, we have to use some heuristics to detect method calls.

So, in my opinion, we have three ways:

  1. Specify this rule only for TS-code
  2. Don't make this rule
  3. Use heuristics and name convention to detect watchers

What do you think?

@binjospookie
Copy link
Sponsor Contributor Author

binjospookie commented Oct 1, 2021

I like the first way. I'll explain why.

Don't take the rule away from developers with TypeScript and Effector projects just because not all projects have TypeScript.

Binding to the name convention seems fragile because any naming rule can be disabled.

@igorkamyshev
Copy link
Member

I agree. But we should notice it in documentation.

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