DevEx: Calling "`readSignal`" with arguments should throw

[danieltroger]

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

1. Go to playground link
2. See that a getter gets passed to a function expecting a setter without any error

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

[fabiospampinato]

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.

[ryansolid]

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 undefined I agree that there should be an issue here via TS.

[fabiospampinato]

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 🤔

[danieltroger]

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

[ryansolid]

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.

[danieltroger]

I cared a lot about the characters so people working TS had to work around that.

You mean the bundle size or the subjective character of signals?

Like it isn't really the libraries responsibility to check for people calling functions with extra arguments.

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.

[ryansolid]

Yeah bundle size.. setting set(undefined) becomes s(void 0) in most modification rather than just s(). I don't care as much now. But historically it was a thing. I'd love to simplify Setter types if it would help in the future. Although I think the TS issue is more complicated than that when reading the conversation.

Yeah let's go with dev warning.

[ryansolid]

I attempted to add this to the repo and it actually caused a bunch of internal bugs. Not too hard to fix. But requires adding function wrappers on createMemo calls mostly. It is interesting because unlike with the write there is no really downside to reading a Accessor read into a createMemo call directly. Which leaves me a bit on the fence here on erroring in this case.

[danieltroger]

Did you attempt the typescript fix (discord link) or the dev warning?

I assume the typescript fix and it makes so that it's illegal to call accessors with arguments? 🤔