solidjs / solid

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

Solid attaches `Symbol(state-node)` and `Symbol(state-proxy)` properties to objects which are unrelated to SolidJS #360

Closed benrbray closed 3 years ago

benrbray commented 3 years ago

Describe the bug

During the course of migrating from Webpack 4 -> 5, a mysterious bug appeared in my appeared which I believe originates from SolidJS. I am using Electron 12.0.0, Node 14.16.0, and SolidJS 0.18.14 with TypeScript 4.2.3 on Ubuntu 20.04.

Here is how I tracked the bug to SolidJS:

Questions

Reproduction

I'll try to create a MWE later if no immediate solution springs to mind, but the setup is rather complicated. For now, you can see the full project webpack setup in this repository, if you like. You can run with npm run dist, then open the executable in the dist folder. Go to File > Open Folder and choose a folder with some text files in it. Open one, and try to save it -- an error should appear in the console.

Some relevant lines of code, for completeness (although I really don't expect you to go digging in this huge unfamiliar codebase):

Expected behavior

Objects I create which are unrelated to SolidJS should not have any additional properties assigned to them that might create problems for serialization.

Thanks in advance for any assistance you can provide! If needed I'll try to update my SolidJS -- I made a quick attempt earlier by following the changelog, but received a ton of errors related to JSX.IntrinsicElement, so I suspect updating will be nontrivial. In the meantime, I'm hoping you might already know what's going on.

ryansolid commented 3 years ago

Yeah these are completely related to Solid. I mean they aren't unrelated if they are in Solid's deeply nested state object. For those to appear they are both in a state object and being read under a reactive tracking context.

Weakmaps are slow/awkward on memory so I was using non-enumerable symbols on state objects. Mostly that Solid needs to own these objects since it needs to control their mutation (can't detect outside changes) so I didn't think it would be too problematic. But yeah you are upgrading form 0.18.4.. a decent amount has changed from then. Changed the whole way imports work to align with modern ESM (aped Preact's setup). Got rid of global polluting JSX definitions. I mean need to go through the change log. Even changed the reactive timing a little bit for better concurrent rendering and for streaming SSR.

I think to best solve this well have to understand the underlying approach and see if we can find a good solution. I think interopt is tricky and we need to find the right patterns here. Might need to examine how to handle Electron in general. Granular reactivity by it's nature is a bit at odds with structured clone approach used by immutable data structures since it needs to keep references to track mutation. I have some tools for interopt but they seem to be not covering this case properly.

I will take a look around a bit but I suspect it might be time to look at the core of the electron integration and see what we can do because I suspect there is a mismatch (from other issues) and Solid has evolved a bit. I think though I've added new primitives to make some aspects better but I think this might take some engineering to solve. And will have to figure out when to best address this.

benrbray commented 3 years ago

Thanks for the reply!

Yeah, the currentFile object actually is being assigned to a state object in order to display some file information to the user.

I will keep trying to work around the issue, but so far the only thing I've found that works is to manually create a shallow copy of the object without the extra symbol keys before I send it over ipc.

ryansolid commented 3 years ago

Yeah... there are a couple of other ways. I probably should look a bit more at what MobX does. They have similar symbol properties on observables. Explicit untrack in scenarios of nested reads won't wrap and won't create proxy and symbols.. a bit tedious to know all the places accessed. Another trick is to use classes since state only tracks/modifies object literals (ones with with Object prototypes). These I suppose are a little brittle. I hope to refine the patterns here as I can work through more scenarios.

ryansolid commented 3 years ago

Just doing some house cleaning. I think this is intended behavior so not sure if there are more actions to take. It is unfortunate that serializing is hitting these.

It occurs to me that I changed the behavior to make them non-enumerable (by using Object.defineProperty) but that probably was after 0.18.4. Did you ever succeed updating to the latest Solid? That might just fix the issue actually with no other changes.

ryansolid commented 3 years ago

I'm going to close this as it has been stale for a while.

benrbray commented 3 years ago

Update: I put off upgrading for a while, since I couldn't find a way to work around this issue. I recently tried again, and was successful upgrading to 0.26.0! The symbols attached to solid-js elements objects don't seem to cause an issue anymore, so thanks for the fix, whether it was intentional or not :).