-
-
Notifications
You must be signed in to change notification settings - Fork 876
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
DevEx: Calling "readSignal
" with arguments should throw
#2045
Comments
IMO it should throw in production also, for consistency. Like if we know we stumbled on an error trying to bail out from the current execution by throwing seems sensible. |
readSignal
" with arguments should throw in solid-js dev modereadSignal
" with arguments should throw
Is this a TS request/error? It seems like it. We use setters with arguments in a number of places internally I believe but that is only for undefinable values. Like if you initialize with a number and the Signal is not able to be set to |
I think the request is for throwing at runtime when calling a getter with more than 0 arguments, so nothing really strictly about typescript types 🤔 |
Exactly this See the discord discussion starting here |
Ok.. that seems reasonable. Obviously, dev mode is no cost. Prod is a different beast. This issue no-ops right now so I wouldn't be opposed to warning in dev rather than erroring. Erroring is stronger and suggests we should break, but then again I don't know that I'd want to put these sort of error checks in many places in general. Like it isn't really the libraries responsibility to check for people calling functions with extra arguments. It would be different validating inputs but its like outside of the scope of operation. I see value in aiding in Dev but it seems oddly forced for prod. But throw worthy error feels it should always throw. This is why I went to TS as that's what its for. The TS issue which I assume is because argumentless setters which admittedly might be a stupid thing. I cared a lot about the characters so people working TS had to work around that. It is just something in that domain. |
You mean the bundle size or the subjective character of signals?
I think that if solid-js wants to gain more adoption it should keep devEx in mind and "close" common traps one may fall into. It's a library many humans will use, it should account for them making mistakes. While this cost me 10 minutes, it may cause someone starting to learn solid to just give up on solid-js entirely, thinking it's "too complicated". But I totally buy that the throw is not worth the bundle size in prod. Just add a warning in dev mode if you think that's the best and I'm happy too. |
Yeah bundle size.. setting Yeah let's go with dev warning. |
Describe the bug
Right now it's possible to do a mistake when destructuring, see this example: https://playground.solidjs.com/anonymous/26548b4b-87ed-4b76-bfbd-2cb6fa2b5b42
This mistake is not caught by TypeScript right now and leads to unexpected behavior - effects being re-executed when nothing happens and my value not being set.
Your Example Website or App
https://playground.solidjs.com/anonymous/26548b4b-87ed-4b76-bfbd-2cb6fa2b5b42
Steps to Reproduce the Bug or Issue
Expected behavior
I as a dev should not have to debug such a basic mistake, my computer should blurt red in my face.
Since it seems like nothing happens with stuff passed to a
readSignal
function right now it would be easy for you to just throw in dev mode, saving people that make this mistake time debugging.Screenshots or Videos
No response
Platform
macOS (M1), latest chrome or firefox
Additional context
No response
The text was updated successfully, but these errors were encountered: