useMutableSource: Allow returning function from getSnapshot #18921
Conversation
basicStateReducer strikes again.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b4cf5b0:
|
|
@sophiebits Does this seem to fix #18823? |
|
@dai-shi It should. |
| @@ -1012,7 +1012,8 @@ function useMutableSource<Source, Snapshot>( | |||
| const latestSetSnapshot = refs.setSnapshot; | |||
|
|
|||
| try { | |||
| latestSetSnapshot(latestGetSnapshot(source._source)); | |||
| const value = latestGetSnapshot(source._source); | |||
| latestSetSnapshot(() => value); | |||
|
|
|||
| // Record a pending mutable source update with the same expiration time. | |||
| const suspenseConfig = requestCurrentSuspenseConfig(); | |||
There was a problem hiding this comment.
(nb: we still need to support some kind of unwrapping because of the exceptions a few lines below this – we could maybe make something different for that to avoid allocating the function in the success case, but… I feel like this is fine?)
|
Do we actually want to support this or is it more about defining a behavior that's currently poorly defined? Using a function as your "snapshot" only works if it's pure/idempotent. It doesn't work if it reads from anything mutable (including the source itself). I can't think of any legit use cases for this. Maybe to make an expensive computation lazy, but that's a subtle thing to get right because you have to pluck out the inputs before closing over them. I would consider that a very advanced use case, and there are other ways to get around it, like by wrapping the computation in an object: As an alternative, could we say we don't support functions as snapshots and warn in dev if we detect one? |
|
@dai-shi's use case of storing a constant function in your store like a state setter (https://github.com/dai-shi/use-context-selector/tree/v2) doesn't seem unreasonable to me, though it's not how I would write the code. But more I noticed this while reading the code and I don't like things that are ostensibly agnostic to the value you put in them except fail on certain ones for no good reason – I assumed it was an oversight. |
| @@ -1012,7 +1012,8 @@ function useMutableSource<Source, Snapshot>( | |||
| const latestSetSnapshot = refs.setSnapshot; | |||
|
|
|||
| try { | |||
| latestSetSnapshot(latestGetSnapshot(source._source)); | |||
| const value = latestGetSnapshot(source._source); | |||
| latestSetSnapshot(() => value); | |||
There was a problem hiding this comment.
This is an issue that I chatted about with Daishi a few weeks back, which has this obvious fix but... I had hoped to avoid the [almost always] unnecessary function wrapper. Still don't know how I feel about it.
There was a problem hiding this comment.
Andrew's comment just loaded in for me after writing mine.
I agree with the concern he mentions about opening up extra problem cases if the function itself closes over mutable values.
As an alternative, could we say we don't support functions as snapshots and warn in dev if we detect one?
I tend to like this better.
|
Another option is to use OTOH, we've talked about making it so Regardless I still kinda prefer not supporting this and warning. |
|
I've posted a potential alternative for discussion #18933 |
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
|
We decided not to support this, but to warn instead (#18933). Just forgot to close the issue. |
basicStateReducer strikes again.