solidjs / solid

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

Static component is being compiled even with state change. #163

Closed ghost closed 4 years ago

ghost commented 4 years ago

In the compiled function for App, instead of a component function, a static component is returned. Is this a bug, or am I doing something wrong?

const App = () => {
  const [state, setState] = createState({
    component: A,
    props: {}
  });
  const r = (path, c) => register(path, (params) => {
    setState({
      component: c,
      props: params
    });
  });
  r('/a/welcome-:name', A);
  setOnFailure(() => setState({
    component: NotFound,
    props: {}
  }));
  route();
  setTimeout(() => route('/b'), 1000);
  return <state.component {...state.props}/>;
};
ryansolid commented 4 years ago

Yes this isn't supported this way since the Component only renders once. To do that we need to wrap the access to the state.component in an computation in your case the easiest way to do that is return a function:

return () => <state.component {...state.props} >

Keep in mind that means when state.component changes it will completely replace the downstream. If that isn't what you want we can see if there are solutions but generally any top level reactivity in the JSX like a conditional etc requires wrapping in a function so it is lazily evaluated. Otherwise it just grabs it once and never gets back to it again.

Generally I haven't found this to be a common pattern but it's worth considering optimizing if it is although that would be possibly deopt for more common things like Context API. I know particularly people from the Svelte community are accustom to this sort of thing. Generally I'm more of a React Router sort of person.

ghost commented 4 years ago

Yes, that's what I thought. Since this function is invoked only once, that's why this is happening.

But isn't there a way to handle this in the compilation step itself? The solution could be, to detect if there is a state change happening anywhere, and in case state change is involved, then return a function/computation instead of a static component.

ryansolid commented 4 years ago

For now I think my advice is to do it the way I highlighted in this issue: https://github.com/ryansolid/solid/issues/73

Here is the updated CodeSandbox with current Solid APIs: https://codesandbox.io/s/solid-dynamic-component-qhmsn?file=/index.js

You'd use it in your example like:

const App = () => {
  const [state, setState] = createState({
    component: A,
    props: {}
  });
  const r = (path, c) => register(path, (params) => {
    setState({
      component: c,
      props: params
    });
  });
  r('/a/welcome-:name', A);
  setOnFailure(() => setState({
    component: NotFound,
    props: {}
  }));
  route();
  setTimeout(() => route('/b'), 1000);
  return <Dynamic component={state.component} {...state.props} />;
};

With compilation you are limited to static analysis. We have some limitations since anything that could be hoisted out of the file I can't assume. So I can't know about state changes. What I can do is use syntax based heuristic to remove what can only be static. My gut feeling is if we want to support this we consider a special syntax. I don't want the period to be the decider since it has other uses.

Namespaces might be useful when TS supports them. Namespaces can't combine with member expressions (you can't $:state.component). Mangling the name without namespace will give TypeScript issues($state.component), basically having to type per location. So thinking maybe we force function accessors like:

const Comp = () => state.component
return <$:Comp {...state.props} />

I'd have to see how it work. I honestly think the control flow I made above might be good enough. I've resisted adding it since it is a 2 liner but maybe it's worth it to remove the confusion. I get asked about this every few months it seems.

ghost commented 4 years ago

Yes. I guess I was thinking with context from react, where the function would be called on each render. Being explicit about the return as static or as computation is better, and helps to build the mental model for solidjs.

Whereas converting the return expression to computation would become a behind the scene magic, and maybe lead to misunderstanding of how solidjs rendering works.

ryansolid commented 4 years ago

Yes but I'm interested in understanding how people would approach these things. Solid is going to be compared to React for a long time given the API and technology choices. For me the idea of componentizing the control flow is very Solid. It takes the Template DSLs of Vue or Svelte and puts that React-like flexibility of just write a component for it over top. Anyone can write a v-for of a svelte:component etc..

ryansolid commented 4 years ago

I think there is no action on this one. So I'm going to close this. Reopen if there is anything else to address here.

ryansolid commented 4 years ago

I added Dynamic to the core with 0.18.8. What won me over is this is a way to do dynamic Intrinsic Elements by string tags.

ghost commented 4 years ago

Great.

But I remembered a bug where, even though state was updated, a component with <div>{state.something}</div> was not being updated as it wasn't getting compiled as <div>{() => state.something}</div>, resulting in a static state.something. Though as you had said earlier that the preferred way is to use arrow syntax for dynamic parts, but it still confused me as, at some places things get automatically compiled to dynamic function and at some places they aren't. Most probably it is a bug. I will try to reproduce it and post it here, if I am successful.

Also, I tried to implement a library similar to Solid, but without the need of compilation step. Maybe it is possible to integrate the idea in solid too. Here is the implementation.

ryansolid commented 4 years ago

Yeah that must have been a bug. Children of DOM elements should always detect. It's possible that you were trying to make a nested Component Dynamic by wrapping the whole thing instead of it individually. I agree that it is confusing so introducing Dynamic removes the need completely.

I regret you stumbled on this gap so early on. I've used reactive libraries for years and never had a need for this pattern. Probably since my earliest library experience didn't support it. For me it's like the 2% edge case but it forced you on path to understand things you probably wouldn't hit for months normally. Still on the bright side, with Dynamic I hope no one else needs to puzzle through this so quickly.

Oh and Solid already can work without a compilation step both with HyperScript and Tagged Template literals. https://codesandbox.io/s/0vmjlmq94v https://codesandbox.io/s/jpm68z1q33

I have the custom compiler since it allows removing the extra explicit wrappers, reduces code size, and improves performance. But the HyperScript version can work with traditional JSX compilation.

I suppose I could make it more obvious in the Readme. This is what it currently says: https://github.com/ryansolid/solid#solid-rendering

EDIT: Nice use of RxJS I actually started there. But decided over time that simpler primitives more suited my goals given the verbosity. At one point between proxies, web components, and TC39 Observable proposal I was thinking I could do something completely based on standards.

ryansolid commented 4 years ago

I've made a header now in the Readme to make it more obvious. Thanks for pointing this out.