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

Extract abstract bindings #11

Open
robinweser opened this issue Jun 14, 2017 · 14 comments
Open

Extract abstract bindings #11

robinweser opened this issue Jun 14, 2017 · 14 comments

Comments

@robinweser
Copy link
Member

I just thought about replacing the theming helpers in fela with this unified approach. Sadly it only supports React and we have to serve something for Preact and Inferno as well.
(robinweser/fela#302)

What do you think about extracting the abstract parts and compose them to several packages such as React, Preact, Inferno and maybe even more?

@kof
Copy link
Member

kof commented Jun 14, 2017

What exactly is needed to support preact and inferno out of the box? I think we all want it and if its not too special it might be ok to keep it all here in this repo.

@kof
Copy link
Member

kof commented Jun 14, 2017

Preact has preact-compat, there should be no need for all this preact-* packages. Something goes wrong if every react package willl create preact- and inferno- packages. cc @developit

@iamstarkov
Copy link
Member

hi @rofrischmann, does abstracting mean that we will have 3 packages, like {react,preact,inferno}-theming?

@robinweser
Copy link
Member Author

robinweser commented Jun 14, 2017

I am no expert in either Preact or Inferno, but I thought that using preact-compat is actually not the most recommended way to use Preact in general, but rather a quick production-replacement for React apps.

Most likely it's just the different createElement import and some minor changes. Check out how we did it with Fela e.g. the createComponent-HOC:

(See how the abstract factory contains all the logic while the actual bindings simply add in the correct APIs)

But I also see @kof's point with different packages for any React lib.

@iamstarkov
Copy link
Member

@developit
Copy link

developit commented Jun 14, 2017

This is a good pattern. If it's useful to you, preact now exports createElement() (same as h()). That means all the major VDOM libs support { createElement, cloneElement, Component, render } - so your factory could just accept that as an object if it's any easier. I wrote a post about the technique and how you can potentially even use a Webpack loaded to accomplish this too.

@kof
Copy link
Member

kof commented Oct 14, 2017

@rofrischmann can you help with this? You are already a collaborator on this project.

@robinweser
Copy link
Member Author

If you’re fine with 3 different packages (themig-* or *-theming where * = react/preact/inferno) I can help here, but after ReactiveConf ofc :p
Could talk there as well

@kof
Copy link
Member

kof commented Oct 14, 2017

What other choices do we have? Can it be a functional way? Like setReactLibrary() or something?

@robinweser
Copy link
Member Author

robinweser commented Oct 14, 2017

Something like this? That would mean, we only ship the abstract factory and the library itself decides what to use?

React

import { createElement, Component } from 'react'

const theming = themingFactory({
  createElement, 
  Component
})

// where theming ships the APIs
const { withTheme, ThemeProvider } = theming

Preact

import { h, Component } from 'preact'

const theming = themingFactory({
  createElement: h, 
  Component
})

// where theming ships the APIs
const { withTheme, ThemeProvider } = theming

(The examples are just for demonstration, might not includes every detail, but shows the idea)

@kof
Copy link
Member

kof commented Oct 14, 2017

Sounds like a good solution! Way better than to produce separate packages or even repositories 😅

@robinweser
Copy link
Member Author

Well it really depends. If this package is only for library authors, the above example is the best solution by far, but if you want to serve direct end-user as well, separate packages for each library are much better in terms of UX/DX => "install & use".
Thanks to tools like Lerna, managing separate packages is pretty much as if its just one.
But I assume theming should only be used by libs anyways so we can use the factory pattern.

@kof
Copy link
Member

kof commented Oct 14, 2017

I assume we target first of all the lib authors. If we keep react by default, most regular users still don't have to do much. Also using a factory is not the end of the world.

@developit
Copy link

developit commented Oct 14, 2017

Just a note: because preact exports createElement in version 7+, you can do this:

import * as preact from 'preact'
const theming = themingFactory(preact)
const { withTheme, ThemeProvider } = theming
import * as react from 'react'
const theming = themingFactory(react)
const { withTheme, ThemeProvider } = theming

That'll have the added benefit of giving you access to cloneElement() in your factory.

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

4 participants