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

Question: Why are you using Solid's createMutable? #498

Closed
orenelbaum opened this issue Jun 18, 2022 · 11 comments
Closed

Question: Why are you using Solid's createMutable? #498

orenelbaum opened this issue Jun 18, 2022 · 11 comments
Assignees
Labels
core Mitosis Core enhancement New feature or request solid Solid.js

Comments

@orenelbaum
Copy link

I wasn't sure if I should open an issue about this but I saw that you are using Solid' createMutable which is not recommended and is intended mostly for easier transition from libraries with similar APIs. Other than not having read and write segregation it also leads to over subscription. Maybe you guys are aware of that already, just wanted to make sure.

@trusktr
Copy link

trusktr commented Jun 18, 2022

createMutable is wonderful. I really love it. Better DX when read/write segregation is not needed (or not desired).

Sometimes the DX is more worth a negligible performance loss. The over subscription only happens in some cases, but Solid has been making progress on this (f.e. Array methods).

@trusktr
Copy link

trusktr commented Jun 18, 2022

Oh, if we're talking about build output though, we may as well use the most optimal output: plain signals and memos whenever possible.

@orenelbaum
Copy link
Author

Yeah we are talking about build output.

@samijaber samijaber added enhancement New feature or request core Mitosis Core solid Solid.js labels Jun 22, 2022
@samijaber
Copy link
Contributor

Yeah, we're definitely open to producing the more idiomatic output!

However, from reading SolidJS docs, it seems like the only other choice is createStore + produce, which does not map as well to our Mitosis createStore. See this Mitosis example:

import { useStore } from '@builder.io/mitosis';

export default function MyComponent(_props) {
  const state = useStore({
    list: ['hello', 'world'],
    newItemName: 'New item',
    addItem() {
      state.list = [...state.list, state.newItemName];
    }
  });

  return (
    <div/>
  );
}

Which currently becomes this SolidJS code:

import { createMutable } from "solid-js/store";

function MyComponent(props) {
  const state = createMutable({
    list: ["hello", "world"],
    newItemName: "New item",
    addItem() {
      state.list = [...state.list, state.newItemName];
    },
  });

  return <div></div>;
}

export default MyComponent;

I'm not super familiar with Solid's state management solutions, but it looks to me like far more complicated logic to convert this into a handful of createStore instances.

How would this above example end up looking if it was idiomatic SolidJS?

@orenelbaum
Copy link
Author

orenelbaum commented Jun 23, 2022

This is the most idiomatic way:

import { createStore } from "solid-js/store";

const [state, setState] = createStore({
  list: ["hello", "world"],
  newItemName: "New item",
});

const addItem = () => setState("list", state.list.length, state.newItemName);

You can put addItem inside the store, I don't know if it's common among Solid users but I don't think there are any problems with that.

You can also do

const addItem = () => setState('list', produce(l => l[l.length] = state.newItemName));

Or less optimally one of those:

const addItem = () => setState('list', [...state.list, state.newItemName]);
const addItem = () => setState(produce(s => s.list = [...state.list, state.newItemName]));

Which will update the whole list instead of just a specific position in the array.

@steve8708
Copy link
Contributor

steve8708 commented Jun 23, 2022

ah yes, alternatively we can also use signals too. what I like about createmutable is support for nested mutation, so similar to what we do for react, svelte, etc we could just have an output using signals (basically identical to our react hooks output except all access is a function call instead of a direct reference)

const [list, setList] = createSignal([...]);
const [newItemName, setNewItemName] = createSignal("New Item")

return <div><input value={newItemName()} ... />...</div>

this would potentially be the easiest path since we can reuse a lot of the logic for react hooks

@orenelbaum
Copy link
Author

Yeah this is the easiest solution and I think that it's also slightly faster than stores although stores are more idiomatic.

@samijaber
Copy link
Contributor

Asked questions in the SolidJS Discord, here's the thread https://discord.com/channels/722131463138705510/751355413701591120/1001192746301788180

We might want to try some of these options:

  • find a way to batch code (while making sure that code does not access the updated values, which will cause stale content to be used)
  • use his createLiveMutable implementation
  • wrap useMutable with a Proxy (as per Fabio's suggestion)

@steve8708
Copy link
Contributor

steve8708 commented Jul 25, 2022

Should we not just use signals? What's the downside to the most simple/idiomatic approach used in solidjs's docs? Everything else here sounds more complex and deviates from the idea of "generate code just like a developer would write" for a given framework

We should be able to just reuse all of our react hooks logic, just make sure all reads add a (), so not list but list() in the code, which is a normal sort of thing we do for other output types

@samijaber samijaber self-assigned this Jul 27, 2022
@trusktr
Copy link

trusktr commented Aug 2, 2022

It is totally conceivable people can write all their state variables using createSignal (the fastest option), so outputting that would be totally fine. f.e.

const [foo, setFoo] = createSignal(...)
const [bar, setBar] = createSignal(...)
const [baz, setBaz] = createSignal(...)
// ...etc...

Maybe less ergonomic than stores in various cases, but still legitimate and still an idiomatic option

@samijaber
Copy link
Contributor

We have added support for createSignal via a generator config a while back: #608

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Mitosis Core enhancement New feature or request solid Solid.js
Projects
Status: Done
Development

No branches or pull requests

4 participants