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

Any ideas for mixin support? #21

Closed
longshorej opened this issue Sep 10, 2014 · 10 comments
Closed

Any ideas for mixin support? #21

longshorej opened this issue Sep 10, 2014 · 10 comments

Comments

@longshorej
Copy link
Contributor

Mixins are useful mostly because of their ability to hook into the component lifecycle and register / deregister things when a component is un/mounted. http://facebook.github.io/react/docs/reusable-components.html

The example on setTimeout() is a good one - a mixin makes methods available to the equivalent of the Backend methods and also hooks into lifecycle methods i.e. componentWillMount.

Not sure if anyone has any ideas on how to implement this, or if it is needed. I would think at first glance it would involve traits and functional composition for the lifecycle methods but can't figure out a nice way to do it. Another idea was maybe an additional "MixinConfiguration" in ReactComponentB to supplement Backend? It could be passed to the Backend's constructor via BackendScope. buildSpec would then have to set the mixins property of the Dynamic object so that its lifecycle methods are called correctly.

Hopefully that made some sense. Just looking for some feedback on the idea. I'll gladly give implementation a go if anyone else is interested.

(quick guess that may not make sense below)

trait Mixin {
  def componentWillUpdate: ...

  ...
}

trait MixinConfiguration {
  def mixins: List[Mixin]
}

class B1[MC <: MixinConfiguration] {
  type ScopeB = BackendScope[Props, State, MC]
}
@dispalt
Copy link
Contributor

dispalt commented Sep 10, 2014

I am interested in mixins. I used a really hacky way to get it. One thing to keep in mind is generally mixins require some functionality to be used on the object passed in to createClass. I never really thought how to do that elegantly.

@japgolly
Copy link
Owner

I'm not really sold on the idea of mixins as an optimal solution to problems. I looked at the setinterval example and it's horribly unsafe and relies on a single namespace per component to be shared between all clients & mixins, fingers crossed that there is never a conflict or a client forgets to set a required variable.

For me to personally work on this I'd like to be convinced with more compelling examples or use cases (please do try) but I'm not sold on it yet. If a solution PR is submitted and people find it useful (how to measure that I'm unsure) then great, I wouldn't use it but I'll merge it in and maintain it. But I am sceptical....

Take the benefit demonstrated in the setinterval mixin example, it's basically reuse for managed state and cleanup on component unmount. If it weren't for the component unmount hook, I'd say just use a function. Considering the hook, yes, I can see what we have currently is lacking and it is a problem to be solved but I would argue against mixins as the solution. The first thing that comes to mind is composability, probably composability of event hooks....

...if the component builder combined rather than replaced event hooks (ie. ReactComponentB.componentWillUnmount could be called twice to install two different unmount handlers) then I believe this problem could be solved simply by something like:

// In place of the mixin, here lives the reusable stuff
class SetIntervalThingy {
  private var ids: List[Long] = Nil
  def add(f: => Unit): Unit = ...
  def unmount(): Unit = ...
}

// Here be its creation and event hook installation
def useSetIntervalThingy(f: SetIntervalThingy => ReactComponentB) = {
  val s = new SetIntervalThingy()
  f(s).componentWillUnmount(_ => s.unmount())
}

// Using in a component
useSetIntervalThingy(s =>
  ReactComponentB[Unit]("Blah").render(...)
).createU

Note: Mixing-in of pure JS components may be separately compelling but is being tracked in issue #8, not here.

@japgolly
Copy link
Owner

Ah just realised that in my example all component instances share the same SetIntervalThingy instance. That would need to change but the concept would be the same.

@longshorej
Copy link
Contributor Author

I think you're right. I was hacking away at this a bit yesterday based on my original idea but it's simply not as good as what you've got above.

...if the component builder combined rather than replaced event hooks (ie. ReactComponentB.componentWillUnmount could be called twice to install two different unmount handlers) then I believe this problem could be solved simply by something like:

I submitted a pull request for this functionality. Hopefully it's up to par :P

@japgolly
Copy link
Owner

I thought about it a little more too and my previous example gets a bit trickier if you want separate mutable state per component instance. Then I realised wait that's exactly what "backends" are for. With a Scala backend you're free to build it however you wish; mixins are already supported via traits.

Thus I'll consider Scala mixins and sharing of that style already supported, and now close this issue.

Feel free to continue to debate or throw ideas around within this issue and if a strong enough argument emerges we can reopen.

@dispalt
Copy link
Contributor

dispalt commented Sep 11, 2014

Yeah I was interested in supporting mixins for the case when I use other people's libraries, which is covered in #8

@longshorej
Copy link
Contributor Author

I was able to implement what I needed with 8b5bad8 and your sample code above.

Note that the type signature looks a bit more crazy than the sample above:

  def useSetIntervalThingy[P, S, B, C](c: SetIntervalThingy => ReactComponentB[P]#B2[S]#B3[B]#B4[C]) = { ... }

@japgolly
Copy link
Owner

japgolly commented Oct 8, 2014

There have been changes lately that make Scala-style component mixins easier to work with. (Sorry still no JS-mixin support).

I've experimentally added three such mixins and examples that use them.

  • OnUnmount - Automatically run stuff when component unmounts.
  • SetInterval - Like window.setInterval but without having to worry about calling clearInterval on unmount.
  • Listenable - A component is able to receive external data. Automatically registers on mount to receive data and unregisters to stop on unmount.

Examples:
https://github.com/japgolly/scalajs-react/blob/master/example/src/main/scala/japgolly/scalajs/react/example/ExperimentExamples.scala

Also types have changed so no more ReactComponentB[P]#B2[S]#B3[B]#B4[C]. More detail here.

See what you think. Cheers.

@longshorej
Copy link
Contributor Author

The type change is nice!

I'm still trying to digest the other stuff. Something feels wrong about having to specify e.g. Listenable twice for a component definition - once in configure and once for the backend. I'll have to look at it more and see if I can vocalize it.

@japgolly
Copy link
Owner

Aye, I had the same feeling as well. The problem is if a mixin is stateful it needs someone to store that state somewhere. I thought of four possibilities:

  1. In the component's state. (Too disruptive, you'd need lenses everywhere.)
  2. In the backend.
  3. Floating in the ether like var a = 0; () => {a+=1; a}. Apart from never being able to inspect it the larger problem is that it would be defined once per component definition which means component instances share it when they're mounted instead of having their own. Which effectively makes it no different than....
  4. A global variable, probably keyed by component instance. Don't like.

A different approach would be for a base mixin which has a bunch of component lifecycle event callbacks which ReactComponentB calls automatically but with that you lose composition. If you combine mixins you need work around inheritance overriding callbacks instead of composing. Plus it starts getting a bit magical; I like things a bit more explicit.

So I know what you mean and shared your intuition but have ended up convincing myself that the backend is the way to go. The .configure(SetInterval.install) bit should be what's committed to memory because it also comes with constraints giving you compile-time info if you forgot to update the backend.

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

No branches or pull requests

3 participants