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

Bug: tear-offs don't trigger computed() #334

Open
wants to merge 1 commit into
base: 6.0
Choose a base branch
from

Conversation

Lootwig
Copy link
Contributor

@Lootwig Lootwig commented Nov 5, 2024

Added a test case illustrating the issue.

@rodydavis
Copy link
Owner

I think this is working as expected. Computed needs to return a value but with a tear off it returns a function.

When would you need to provide a tear off like that?

@Lootwig
Copy link
Contributor Author

Lootwig commented Nov 10, 2024

My understanding was that using a tearoff would be equivalent to an explicit call - after all, the input to computed() is still very much an instance of

  • something callable
  • taking no inputs
  • generating no output (or output is simply ignored)

I am obviously wrong about the "equivalence" part of that assumption, but if the criterion for supporting these is literal need, that point is irrelevant anyway.

In that case, however, some type of hint about the unsupported use case would be highly appreciated - ideally an assertion causing the call to fail or an analysis error. That way, others could be spared from first having to realize this is not an issue with the computation content, which in my case was what I initially focused on attempting to debug. The fact that this currently does not result in an error, but just a lack of signal updates, also feels like it could easily just go unnoticed and end up in deployed code. (looks like ImplicitCallTearoff might be the corresponding AST class to trigger on)

@rodydavis
Copy link
Owner

I think it would be a great addition to signals_lint for sure.

The reason it needs to be called is that is when the dependencies are discovered and connected to other signals.

With a tear off it just says that it will run but not actually computed.

There are times you do want a computed to return a function but your example is a good case to add a lint for.

Also to double check you can always test the JS equivalent to see if it is a bug or actual issue too:

https://www.npmjs.com/package/@preact/signals-core

@Lootwig
Copy link
Contributor Author

Lootwig commented Nov 11, 2024

Hah I have to admit it is quite the feeling when you open the codebase of a package written in an entirely different language but feeling right at home because the APIs are completely identical ❤️

@rodydavis
Copy link
Owner

That was the goal 💙

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

Successfully merging this pull request may close these issues.

2 participants