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

upgrade Show component - added prop checkObjectValues #2340

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

madaxen86
Copy link

First of all - this PR is open for discussion

Before adding changes, documentation, etc. I just wanted to get some feedback if this has a chance to be merged.

-- no breaking change --

Show will just behave as before when checkObjectValues is not set / true.
Motivation: e.g.From Discord

Setting it to true will allow narrow the type of all values to NonNullable.
Imagine this case where you have a signal and store that relies in user input without initial values and you need to pass them to a child component which has strict non-null types.
With current Show typescript will complain that first and last might be undefined. So you either have to use the bang operator or use createMemo to narrow like

const narrow = createMemo(()=> {
  if (selected() && store.data?.first && store.data?.second) return {selected:selected(),first:store.data?.first, second:store.data?.second}
  return false
}

And pass it to Show which now correctly narrows to non-null types.

With the added prop you can just:

const [selected,setSelected] = createSignal<Date|null>(null);
const [store,setStore] = createStore<{data?:{first?:Date,last?:Date}}>({})

return <Show when={{selected:selected(),first:store.data?.first, second:store.data?.second}} checkObjectValues>
// children will only render if all values are truthy 
// item is now narrowed to {selected:Date, first:Date,second:Date}
   {item => <StrictChild {...item} />}
</Show>

Summary

How did you test this change?

Copy link

changeset-bot bot commented Oct 17, 2024

⚠️ No Changeset found

Latest commit: 7f0aaa6

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

@madaxen86
Copy link
Author

@davedbase you're also quite active on discord. What are your thoughts on this?

@davedbase
Copy link
Member

@davedbase you're also quite active on discord. What are your thoughts on this?

That would be quite handy tbh.

@madaxen86
Copy link
Author

So,...
I dug deeper and tested a lot, realizing that the with the first implementation, granular updates ware lost. To fix that I implemented a store in the case the checkObjectValues prop is set to true, if false the store won't be created.
So we get fine grained updates for the cost of having createStore and reconcile as dependencies.
But we get closer to type narrowing like in React.
Few questions:

  1. Is it worth to proceed and invest more time, or does this not have a chance to be merged. Then I'll remove the PR. @ryansolid and @davedbase

  2. With latest commit it is still possible to get the callback as an accessor or 'keyed'. But there's really no point of accessing a store. So we could always just return the store. But this would then make the keyed prop obsolete when checkObjectValues is set, because it doesn't change anything. So we have 3 options
    2a. Keep the current implementation
    2b. always return the store
    2c. remove the checkObjectValues prop and add it as an option of keyed keyed:boolean | "checkObjectValues"

  3. if 1. true :-P I would add documentation and also implement the same options for Switch/Match

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants