solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.37k stars 925 forks source link

Calling unwrap on a Solid store returns extra metadata #1106

Closed zaydek closed 2 years ago

zaydek commented 2 years ago

Describe the bug

Calling unwrap on a Solid store returns an object that is decorated with extraneous metadata that is unimportant for the end-programmer.

Here's a minimal repro with a screenshot attached to demonstrate:

Screen Shot 2022-07-09 at 6 30 52 PM

If possible, I'd like the returned object to not include the symbol or have a second argument flag, e.g. deep: boolean, for unwrap API so that metadata is cleared. This is simply a quality of life feature but it would make debugging Solid stores easier without the need to call JSON.parse(JSON.stringify(state)) or equivalent.

Your Example Website or App

https://playground.solidjs.com/?version=1.4.1#NobwRAdghgtgpmAXGGUCWEwBowBcCeADgsrgM4Ae2YZA9gK4BOAxiWGjIbY7gAQi9GcCABM4jXgF9eAM0a0YvADo1aAGzQiAtACsyAegDucAEYqA3EogcuPfr2ZCouOAGU0Ac2hqps+YpU6DW09CysrG24+AUc4ZzdcbjgsXnoIQ0YoQl85BWVVYN0DMkShFXCIZloIEt5gEviUsjhcV1x4gF1eAF4HJxc2pIAKECteWVpaRHyTKEYVLCtJAEoKqpr1OAA6NVoPIbSMrKGGl2XViCt9a-1ZNOZcNGreAGEGCBdGIeX+K5vxhzVWrAKppXBNFpvMFdXqxeLuLxQNRDAAMy0sEBut3G61qGFi8A+PV43x6AD5eM1cFCPkNQbSfgBqXgARnRf2uykx-3GQlwTAgJI52IBAB4TPRcIlBQRiN0VBKpdUVLxqi8NMwANbdED4oSE3CSMnCgGmkD03DfSQm02i-SK6XG7mc8bs523a3urlYwTCMRfUndCmimmfXj6MkpES0Zj0A1bDwtACiajgBoAQvgAJIiIYqLKEFTLACEbrAkg6QA

Steps to Reproduce the Bug or Issue

  1. Open this playground
  2. Open the debugger or console and inspect the loggged object

Expected behavior

As a user I am expecting to receive back the state, clear of metadata or have a second argument flag provided by the unwrap API if for some reason this metadata is important.

Screenshots or Videos

No response

Platform

All

Additional context

No response

ryansolid commented 2 years ago

Currently that is just the way it works. Mostly for performance. I've intended to review this again when Weakmap performance improved. But generally speaking we can't remove this data the way things work as it is the same reference. All we could do is deep clone. Generally these properties are unique symbols and not iterable which is sufficient for most things. I will be doing some performance testing for 1.5 and we can review its impact.

zaydek commented 2 years ago

OK thank you. Honestly I'm having a lot more success modeling state using signals and if I need to instance them per component I think I can just nest them to a component and use a context. In my experiments using stores I found rapidly debugging state a pain point which is when this issue cropped up.

My primary concern is that unwrapped stores are going to appear noisy and potentially hurt user adoption. As a user I just feel uncomfortable using JSON.parse(JSON.stringify) in development to debug state (as I should be). Deep cloning can always be implemented in user-land but I'm sure whatever you have in mind will be faster than JSON.parse(JSON.stringify).

ryansolid commented 2 years ago

This has come up before when the properties were iterable. Since then there hasn't been any reports since most things that serialize will skip those properties. For the most part you can ignore those extra symbols, but I agree debugging proxies in general is a bit of a pain. Stores aren't too bad because you can just look at the target, but prop proxies that come from mergeProps and splitProps are especially awkward because of the indirection (they don't reflect their properties on the proxy target). I know there is some ability to affect at least chromium dev tools display of proxies which might ultimately be our solution for dev tools. That being said Stores generally are too valuable to avoid.

zaydek commented 2 years ago

Thanks for the reply. I'm looking forward to playing with stores again in the future if the developer experience can be slightly enhanced. Seeing as I find proxies somewhat of a hassle in the first place I think I'll continue playing with modeling state just using signals.

This isn't 100% relevant but I wanted to add here as context as to my leaning on signals: The reason I like signals so much is that once you get past the accessor syntax, state can be modeled incrementally without the need for refactoring or implementing complex types. The downside with signals is that if you want to author reusable components (so that instances of a component don't share identical state) you need some mechanism to propagate signals to child components either via prop-drilling or contexts. Still working out how to solve for that but maybe this helps.
zaydek commented 2 years ago

Alright this is working great for me, definitely the debugging experience I was expecting. ☺️

function deepClone(ref: any) {
    if (typeof ref !== "object") {
        return ref
    } else {
        const ref2 = Array.isArray(ref) ? [] : {} as Record<string, any>
        for (const key in ref) {
            if (typeof ref === "object") {
                ref2[key] = deepClone(ref[key])
            } else {
                ref2[key] = ref[key]
            }
        }
        return ref2
    }
}
ryansolid commented 2 years ago

I have to admit I don't understand the complaint still. Debugging is one place where having access seems good. You can go back and forth between the target and the proxy and see what's going on. With a Weakmap you wouldn't be able to do that.

All that being said Weakmaps are still a performance hit. Not as bad as I thought they'd be only a couple points in the JS Framework Benchmark and the majority of the cost in clear rows benchmark. So still inclined to leave as is for 1.5. Maybe I will move this to the discussions to see if we can stir up more conversation. But otherwise I think this issue is stale for now.