solid icon indicating copy to clipboard operation
solid copied to clipboard

feat: call components with `.call` to allow libs to provide class components with static `.call` methods

Open trusktr opened this issue 3 years ago • 8 comments

Also formatted with prettier, some unformatted code got by in previous commits.

Summary

Thanks @fabiospampinato for ideating and ending with this idea.

Calling components as Comp.call(Comp, props) allows for classes to be used as components if they have a static call method. Existing function components work the same, and this is what a class could look like:

class ShowCount {
  static call(Comp, props) {
    return new Comp().template(props)
  }

  foo = 123

  template(props) {
    return <div>Count is {props.count} and foo is {this.foo}</div>
  }
}

function NonClassComp() {
  return <ShowCount count={456} />
}

How did you test this change?

trusktr avatar Oct 01 '22 19:10 trusktr

Pull Request Test Coverage Report for Build 3165847540

  • 1 of 2 (50.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.928%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/solid/src/render/component.ts 0 1 0.0%
<!-- Total: 1 2
Totals Coverage Status
Change from base Build 3147697747: 0.0%
Covered Lines: 1257
Relevant Lines: 1342

💛 - Coveralls

coveralls avatar Oct 01 '22 19:10 coveralls

Maybe it can be just Comp.call(null, props), and a class can use this, as in

class ShowCount {
  static call(_, props) {
    return new this().template(props)
  }
  ...
}

which leads to the same result.

Maybe Comp.call(Comp, props) is better, allows the class author to pick using the first arg or not.

trusktr avatar Oct 01 '22 19:10 trusktr

I can't build or test locally, I get

 ERROR  workspace configuration error: package.json: no workspaces found. Turborepo requires npm workspaces to be defined in the root package.json

Also GitHub actions reports the tests to have passed, but if you take a look, it had a bunch of errors.

trusktr avatar Oct 01 '22 19:10 trusktr

this change may have performance implications, where the common case of "function" has to go through the slower path of "class".

func.call() vs func()


alternatively, the class can be wrapper in a "functional factory". with minimal overhead (<1%), and without any internal changes.

https://playground.solidjs.com/?hash=437964492&version=1.4.1

comparing wrapped/direct instantiating of a class. image

LiQuidProQuo avatar Oct 01 '22 20:10 LiQuidProQuo

That's interesting that a JS engine doesn't simply optimize away what are essentially identical calls.

alternatively, the class can be wrapper in a "functional factory". with minimal overhead (<1%), and without any internal changes.

What would the code look like? If we use new, then it means we must also write code to detect if a function is newable.

trusktr avatar Oct 03 '22 06:10 trusktr

What would the code look like? If we use new, then it means we must also write code to detect if a function is newable.

I have attached a playground to demonstrate what it looks like practically. there is no magic on the framework side, no auto detecting. it is just a way to comply with the current contract of component as a function.

LiQuidProQuo avatar Oct 03 '22 17:10 LiQuidProQuo

Performance implications means it needs to be tested, considered, and not a quick merge.

ryansolid avatar Oct 17 '22 17:10 ryansolid

As I love classes as components I have no idea how I missed this PR 😅 But it seams like the issues stopped being relevant as it can be simply solved by splitting class state from template:

class State {
  foo = 123
}

export function ShowCount(props) {
  const state = new State(props);
  return <div>Count is {props.count} and foo is {state.foo}</div>
}

the flexibility of current solution is strong enough to cover all possible cases and abstract layers without creating new API and deal with perf choices. It also allows for state / template separation for those against SFC. I agree it could be simplified with some utils but I believe that's a role for external package to come up with things like:

import { component } from "solid-class-components"

class State {
  foo = 123;
}

export const ShowCountState = component(State, (props) => (
  <div>
    Count is {props.count} and foo is {this.foo}
  </div>
));

Ofc, this would affect performance as well, but it is fully controllable by the user without general cost to all components.

Exelord avatar Jun 10 '23 09:06 Exelord