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

Fix issue with keepLatest and empty values #1051

Merged
merged 6 commits into from
Jan 5, 2024

Conversation

wagenet
Copy link
Contributor

@wagenet wagenet commented Dec 11, 2023

tl;dr:

  • use the value on the first run of the keepLatest, if available (instead of previous, which is always undefined)
  • add tests clarifying behavior

The way to provide a default value when the value of keepLatest is false is to use another getter.

This PR adds a couple tests clarifying that behavior:

class Demo {
  @use latest = keepLatest({
    value: () => this.data.value?.meta,
    when: () => this.data.isLoading,    
  })

  get theData() {
    return this.latest ?? 'default value';
  }
}

It's important to not do

   value: () => this.data.value?.meta ?? {},

because that means falsey responses (on data), will resolve to {}, and cause content flashes.

So to keep the latest value, you want to let the data response be what it actually is.

In traditional logic, this could maybe be expressed this way:

value ?? default ?? previous

when default is always truthy
vs

(value ?? previous) ?? default

Copy link

stackblitz bot commented Dec 11, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

changeset-bot bot commented Dec 11, 2023

⚠️ No Changeset found

Latest commit: 7970e89

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@wagenet wagenet marked this pull request as draft December 11, 2023 23:23
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review January 5, 2024 18:49
@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label Jan 5, 2024
@NullVoxPopuli NullVoxPopuli merged commit c737cbf into NullVoxPopuli:main Jan 5, 2024
26 of 28 checks passed
@github-actions github-actions bot mentioned this pull request Jan 5, 2024
NullVoxPopuli added a commit to universal-ember/reactiveweb that referenced this pull request Jan 5, 2024
NullVoxPopuli added a commit to universal-ember/reactiveweb that referenced this pull request Jan 5, 2024
* Fix issue with initial values in `keepLatest` and clarify behavior

Copied over from: NullVoxPopuli/ember-resources#1051

Co-Authored-By: Peter Wagenet <[email protected]>

* Update jsTest

* lint:fix

---------

Co-authored-by: Peter Wagenet <[email protected]>
@wagenet wagenet deleted the fix-keep-latest-empty branch January 10, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants