search

salsita / spicy-hooks

7 stars 0 forks source link

Make use of `undefined` for `useSnapshot` #24

Open goce-cz opened 4 years ago

goce-cz commented 4 years ago

Currently the useSnapshot hook returns [null, SnapshotState.WAITING, null] while waiting for the first emit, unless a default value is specified. Using null as a indicator of a missing value feels a bit weird in this case and has some technical implications as well:

Additionally we use null for indication of no error and also for a missing state (whenever the source observable is nullish). I feel that if we convert one, we should probably convert all of them.

So what do we do?

  1. replace null with undefined for value (the first element of the tuple)
  2. replace null with undefined for state in case of a missing source observable (the middle element of the tuple)
  3. replace null with undefined for error (the last element of the tuple)
  4. remove const [value] = useSnapshot(observable, defaultValue) overload in favor of const [value = defaultValue] = useSnapshot(observable)

Note that any of these changes will also affect usePartialSnapshot and useAsyncMemo.

goce-cz commented 4 years ago

I'd definitely do 1 - 3 , but I'm not entirely sure about 4.

craig-bishell commented 4 years ago

I'd also be in favor of 1-3. I feel like Javascript undefined (i.e. no value assigned) is a better fit for the scenario than null (i.e. "empty" value assigned). In terms of 4, I'm not sure I'm qualified to comment... it would depend a fair bit on where useSnapshot (and the other affected hooks) are used and how.

goce-cz commented 4 years ago

One argument for doing no. 4 as well: https://github.com/salsita/configurator-sdk/pull/247#discussion_r503827444

HankRoughknuckles commented 3 years ago

Similar to what @craig-bishell said, I'm not sure how 4 will affect other things, but I think it feels less smelly to do it that way.

In which case, I'm in favor of all of them (1-4)