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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

trusktr
Copy link
Contributor

@trusktr trusktr commented Oct 1, 2022

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?

…ponents with static `.call` methods

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

Co-authored-by: Fabio Spampinato <spampinabio@gmail.com>
@coveralls
Copy link

coveralls commented Oct 1, 2022

Pull Request Test Coverage Report for Build 3165847540

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 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%
Totals Coverage Status
Change from base Build 3147697747: 0.0%
Covered Lines: 1257
Relevant Lines: 1342

💛 - Coveralls

@trusktr
Copy link
Contributor Author

trusktr commented Oct 1, 2022

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
Copy link
Contributor Author

trusktr commented Oct 1, 2022

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.

? P
: T extends keyof JSX.IntrinsicElements
? JSX.IntrinsicElements[T]
: Record<string, unknown>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just formatting applied by prettier

| keyof JSX.IntrinsicElements
| Component<any>
| (string & {});
export type ValidComponent = keyof JSX.IntrinsicElements | Component<any> | (string & {});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just formatting applied by prettier

@LiQuidProQuo
Copy link
Contributor

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

@trusktr
Copy link
Contributor Author

trusktr commented Oct 3, 2022

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.

@LiQuidProQuo
Copy link
Contributor

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.

@ryansolid
Copy link
Member

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

@Exelord
Copy link
Contributor

Exelord commented Jun 10, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants