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

Make use of undefined for useSnapshot #24

Open
goce-cz opened this issue Oct 12, 2020 · 4 comments
Open

Make use of undefined for useSnapshot #24

goce-cz opened this issue Oct 12, 2020 · 4 comments
Assignees
Labels
help wanted Extra attention is needed weirdness Something is Confusing or unclear
Milestone

Comments

@goce-cz
Copy link
Collaborator

goce-cz commented Oct 12, 2020

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:

  • We cannot assign a default value during destructuring const [value = 5] = useSnapshot(observable) as null will override the default. Because of that we need to accept defaultValue as an argument to useSnapshot.
  • It's hard to differentiate between no emission and null emission. undefined would suffer from the same issue, but who does emit undefined?

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 goce-cz added help wanted Extra attention is needed weirdness Something is Confusing or unclear labels Oct 12, 2020
@goce-cz
Copy link
Collaborator Author

goce-cz commented Oct 12, 2020

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

@craig-bishell
Copy link

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
Copy link
Collaborator Author

goce-cz commented Oct 13, 2020

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

@goce-cz goce-cz added this to the v2.0.0 milestone Oct 29, 2020
@goce-cz goce-cz modified the milestones: v2.0.0, v3.0.0 Nov 6, 2020
@HankRoughknuckles
Copy link

HankRoughknuckles commented Feb 28, 2021

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed weirdness Something is Confusing or unclear
Projects
None yet
Development

No branches or pull requests

6 participants