solid-primitives destructure: Added new config option "normalize" #525

madaxen86 · 2023-10-09T18:40:23Z

Added new config option "normalize"
If true all values will be returned as reactive functions, meaning it will resolve values of type MaybeAccessor to Accessor.
This helps a lot for library devs to provide a flexible interface to users and don't worry about having to make breaking changes later on and the handling of the destructured props is always as Accessor.

Maybe it would also make sense to make this the default behavior, but that would be a breaking change.
Test for spread object smart passes.

Added new config option "smart" If true all values will be returned as reactive functions, meaning it will resolve values of type MaybeAccessor<T> to Accessor<T>. This helps a lot for library devs to provide a flexible interface to users and don't worry about having to make breaking changes later on and the handling of the destructured props is always as Accessor. About the naming feel free to rename it to anything you like. I was thinking about: normalize normalizeValues accessibleValues ... Maybe it would also make sense to make this the default behavior, but that would be a breaking change.

Added new config option "smart" If true all values will be returned as reactive functions, meaning it will resolve values of type MaybeAccessor to Accessor. This helps a lot for library devs to provide a flexible interface to users and don't worry about having to make breaking changes later on and the handling of the destructured props is always as Accessor. About the naming feel free to rename it to anything you like. I was thinking about: normalize normalizeValues accessibleValues ... Maybe it would also make sense to make this the default behavior, but that would be a breaking change.

changeset-bot · 2023-10-09T18:40:26Z

🦋 Changeset detected

Latest commit: b56f253

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@solid-primitives/destructure	Minor

Not sure what this means? Click here to learn what changesets are.

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

madaxen86 · 2023-10-09T19:23:13Z

I forgot to change the version in package.json @thetarnav

thetarnav · 2023-10-09T21:28:40Z

Don't change the version. This does it: https://github.com/solidjs-community/solid-primitives/actions/workflows/release.yml
You can just add a changeset with pnpm changeset to describe what changed, or I'll do it before merging

madaxen86 · 2023-10-11T08:14:13Z

I have changed the option to "normalize" and also added the changeset.

it will now return all functions as they are and only memo static values. What I have realized is that once normalized is set to true, it doesn't matter if memo is true or false. The results are the same. So I decided to make it an option of memo. And instead of using exclusive types for the normalize config I extended the existing types to return a different type depending on the memo option.

atk · 2023-10-26T10:24:53Z

Please document the option in the README.md. Thanks!

thetarnav · 2023-10-26T10:39:48Z

And add yourself to the contributors in package.json :)

added myself as contributor in package.json

…into main

thetarnav · 2023-10-26T12:25:40Z

hmm looks like I’ve missed the change when normalize got combined with memo. This doesn’t allow for normalize and memo to be both enabled, why?

madaxen86 · 2023-10-26T12:53:18Z

When I experimented with different config combinations, I realised that as soon as normalised was enabled it didn't matter if memo was true or false.

The returned values were the same, so I decided to merge it with the memo prop to avoid confusion.

I also discovered that the returned types were not correct for all combinations.

That is all fixed now and also checked by the test I have added.

What it does now it the end is memo static values and returning functions as they are. So the memo config option and now works like charm with all the other options 🤓

packages/destructure/test/destructure.test.ts

packages/destructure/src/index.ts

I have added tests to check for the combination of {normalize:true} vs {normalize:true,memo:true} removed the check for memo in the getter function. Moved the check if source is a function outside the getter but it still needs to be a function, otherwise tests fail. Wrapped it in a memo for optimization.

madaxen86 · 2023-10-27T20:08:53Z

Please review the latest commit.
Added tests for the comments and made adjustments accordingly.

thetarnav

Ok, besides the reactive callback stuff that I still need to think about and some minor stuff to correct and it can be merged I think.

thetarnav · 2023-10-28T11:01:59Z

packages/destructure/src/index.ts

-      ? (key: any) => () => source()[key]
-      : (key: any) => () => source[key];
+
+  const _source = createMemo(() => (typeof source === "function" ? source() : source));


whats the reason for memoizing the source? in most cases the primitive is used with props object or a signal, where the source memoization is useless.

packages/destructure/test/destructure.test.ts

thetarnav · 2023-10-28T11:07:33Z

packages/destructure/test/destructure.test.ts

@@ -246,6 +247,142 @@ describe("destructure", () => {
      expect(updates.b).toBe(2);
      expect(updates.c).toBe(2);

+      dispose();
+    }));
+  test("spread object normalize and deep", () =>


You can spit it to two different tests as you are testing two things. "@thetarnav's stuff" and "@madaxen86's stuff". Could be more descriptive btw 😄
I'm aware what my test is doing (testing combining with the memo option enabled and disabled), so I'm guessing the second test is for a function source and deep: true?

packages/destructure/src/index.ts

thetarnav · 2023-10-28T11:59:31Z

packages/destructure/test/destructure.test.ts

+      const [_numbers, setNumbers] = createSignal({
+        a: 3,
+        b: () => (toggle() ? 2 : 3),
+        c: (a: number, b: number) => a * b,


if you replace it with c: () => (a: number, b: number) => a * b the types break, but tests still pass

I have adjusted the types.
It became a bit messy but now the types are inferred correctly.

thetarnav · 2023-10-28T12:01:49Z

packages/destructure/src/index.ts

-    else result[key] = memo ? createMemo(calc, undefined, options) : calc;
+    else
+      result[key] =
+        memo && (!config.normalize || calc.length === 0)


There is one thing I'm realizing about this.

const [callback, setCallback] = createSignal((a, b) => a + b) const {cb} = destructure({get cb() { return callback() }}) setCallback(() => () => 69) expect(cb()).toBe(69) // fail: still calls the original function

Even if we want to avoid cb()(1, 2) syntax of normal destructure, maybe it should still support "reactive callbacks", the same props.callback(1, 2) would if you passed callback={someSignal()} in JSX.
Although Solid doesn't allow for reactive callbacks in on___ props, so it may be fine to restrict that as well.

Co-authored-by: Damian Tarnawski <gthetarnav@gmail.com>

splitted tests for normalize

…into main

thetarnav · 2023-10-29T11:14:17Z

Another question about the callbacks.
How is it supposed to resolve this situation?

type Props = {
	handleClick: (e: MouseEvent) => void
}
const logMousePos = (e: MouseEvent) => {
	console.log("mouse", e.x, e.y)
}

// fine
const a = destructure<Props>({handleClick: logMousePos})

a.handleClick(e) // => works

// variadic params
const log = (...a) => {console.log(...a)}

// ts doen't mind
const b = destructure<Props>({handleClick: log}) // logs immediately

b.handleClick(e) // runtime error - tried to call undefined

madaxen86 · 2023-11-05T18:54:58Z

Another question about the callbacks. How is it supposed to resolve this situation?

type Props = {
	handleClick: (e: MouseEvent) => void
}
const logMousePos = (e: MouseEvent) => {
	console.log("mouse", e.x, e.y)
}

// fine
const a = destructure<Props>({handleClick: logMousePos})

a.handleClick(e) // => works

// variadic params
const log = (...a) => {console.log(...a)}

// ts doen't mind
const b = destructure<Props>({handleClick: log}) // logs immediately

b.handleClick(e) // runtime error - tried to call undefined

Good call.
This is because the 'access' function of @solid-primitives/utils does not check for variadic params (it only does !func.length
I created a new function 'getNormalizedValue' which fixes this issue and also added a test for any kind of combination with variadic params (including nested inside a function without params etc.).
Question:
In this line of the getter

if (typeof accessedValue() === "function" && hasParams(accessedValue())) return accessedValue();

shouldn't we only return if the config normalize:true ?
Otherwise we create a breaking change.

madaxen86 · 2024-01-19T06:48:51Z

@thetarnav Any chance this moves forward or should I submit it as a new primitive?

thetarnav · 2024-01-19T09:00:20Z

I'm sorry for not responding. Looking back at it, I'm still not convinced that this direction is the right one. Checking whether something is a signal or not goes against Solid's principles, it is intentionally designed to make such determinations difficult. When you need to resort to stringifying and testing a function's definition with regex look for keywords more common in callbacks than signals, it's a strong indicator that this may not be the best approach. It's not accurate, it's slow, and it's trying to be too clever.

Unfortunately, the alternative I have doesn't satisfy me either. Solid's compiler treats props starting with on___ as callbacks (like onClick, on:click, oncapture:click, etc.), and doesn't allow them to be reactive. So when destructuring props, we might assume that anything not starting with "on" could be a signal, but that is not foolproof. This heuristic doesn't apply to stores, which could also be used with destructure, and callbacks without the "on" prefix will still be broken.

I'm hesitant to introduce something that users won't have the convenience that it will not cause more problems, when the only one that they wanted to solve was less typing.

madaxen86 and others added 3 commits October 9, 2023 17:34

added test for smart option

ff9b2b9

madaxen86 added 2 commits October 11, 2023 11:47

Changed option "smart" to "normalize"

5c37513

added changeset for option normalize in destructure package

484827d

madaxen86 changed the title ~~solid-primitives destructure: Added new config option "smart"~~ solid-primitives destructure: Added new config option "normalize" Oct 18, 2023

thetarnav and others added 3 commits October 26, 2023 12:40

patch -> minor

c693954

documented option in Readme.md

150b1df

added myself as contributor in package.json

Merge branch 'main' of https://github.com/madaxen86/solid-primitives …

8cf7040

…into main

thetarnav requested changes Oct 26, 2023

View reviewed changes

packages/destructure/test/destructure.test.ts Outdated Show resolved Hide resolved

packages/destructure/src/index.ts Outdated Show resolved Hide resolved

packages/destructure/src/index.ts Outdated Show resolved Hide resolved

madaxen86 and others added 2 commits October 26, 2023 21:07

Merge branch 'main' into main

15438fc

thetarnav reviewed Oct 28, 2023

View reviewed changes

madaxen86 and others added 4 commits October 28, 2023 23:03

Update packages/destructure/src/index.ts

9717358

Co-authored-by: Damian Tarnawski <gthetarnav@gmail.com>

corrected returned type for normalized option.

98f999e

splitted tests for normalize

Merge branch 'main' of https://github.com/madaxen86/solid-primitives …

2159512

…into main

removed unused imports

4d0e264

madaxen86 added 2 commits October 29, 2023 22:29

fix: access value also checks for variadic params

f4eb72f

fix: only return hasParams true of top-level function has params.

b56f253

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

solid-primitives destructure: Added new config option "normalize" #525

solid-primitives destructure: Added new config option "normalize" #525

madaxen86 commented Oct 9, 2023 •

edited

changeset-bot bot commented Oct 9, 2023 •

edited

madaxen86 commented Oct 9, 2023

thetarnav commented Oct 9, 2023 •

edited

madaxen86 commented Oct 11, 2023 •

edited

atk commented Oct 26, 2023

thetarnav commented Oct 26, 2023

thetarnav commented Oct 26, 2023

madaxen86 commented Oct 26, 2023

madaxen86 commented Oct 27, 2023

thetarnav left a comment

thetarnav Oct 28, 2023

thetarnav Oct 28, 2023

thetarnav Oct 28, 2023

madaxen86 Oct 28, 2023

thetarnav Oct 28, 2023

thetarnav commented Oct 29, 2023 •

edited

madaxen86 commented Nov 5, 2023

madaxen86 commented Jan 19, 2024

thetarnav commented Jan 19, 2024 •

edited

solid-primitives destructure: Added new config option "normalize" #525

Are you sure you want to change the base?

solid-primitives destructure: Added new config option "normalize" #525

Conversation

madaxen86 commented Oct 9, 2023 • edited

changeset-bot bot commented Oct 9, 2023 • edited

🦋 Changeset detected

madaxen86 commented Oct 9, 2023

thetarnav commented Oct 9, 2023 • edited

madaxen86 commented Oct 11, 2023 • edited

atk commented Oct 26, 2023

thetarnav commented Oct 26, 2023

thetarnav commented Oct 26, 2023

madaxen86 commented Oct 26, 2023

madaxen86 commented Oct 27, 2023

thetarnav left a comment

Choose a reason for hiding this comment

thetarnav Oct 28, 2023

Choose a reason for hiding this comment

thetarnav Oct 28, 2023

Choose a reason for hiding this comment

thetarnav Oct 28, 2023

Choose a reason for hiding this comment

madaxen86 Oct 28, 2023

Choose a reason for hiding this comment

thetarnav Oct 28, 2023

Choose a reason for hiding this comment

thetarnav commented Oct 29, 2023 • edited

madaxen86 commented Nov 5, 2023

madaxen86 commented Jan 19, 2024

thetarnav commented Jan 19, 2024 • edited

madaxen86 commented Oct 9, 2023 •

edited

changeset-bot bot commented Oct 9, 2023 •

edited

thetarnav commented Oct 9, 2023 •

edited

madaxen86 commented Oct 11, 2023 •

edited

thetarnav commented Oct 29, 2023 •

edited

thetarnav commented Jan 19, 2024 •

edited