matthewp / haunted

React's Hooks API implemented for web components πŸ‘»
BSD 2-Clause "Simplified" License
2.61k stars 92 forks source link

LitElement Support? #87

Closed jdin closed 4 years ago

jdin commented 5 years ago

Hi @matthewp

First of all thanks a lot for your library. I am using it now in my project and I see a great potential.

I have a question / proposal.

I wanted to combine haunted with LitElement but of course it does not work if you just do something like:

component(() => html``, LitElement)

(That was my first attempt :)).

Then I have studied the source code to understand the logic and came up with the following:

// lit-component.js
export {LitElement} from 'https://unpkg.com/lit-element@^2.1.0/lit-element.js';
import {effectsSymbol, hookSymbol} from "./symbols";
import {clear, setCurrent} from "./interface";

class LitContainer {

    constructor(renderer, host) {
        this.renderer = renderer;
        this.host = host;
        this[hookSymbol] = new Map();
    }

    update() {
        this.host.requestUpdate();
    }

    render() {
        setCurrent(this);
        const result = this.renderer.call(this.host, this.host);
        clear();
        return result;
    }

    runEffects(symbol) {
        let effects = this[symbol];
        if (effects) {
            setCurrent(this);
            for (let effect of effects) {
                effect.call(this);
            }
            clear();
        }
    }

    teardown() {
        let hooks = this[hookSymbol];
        hooks.forEach((hook) => {
            if (typeof hook.teardown === 'function') {
                hook.teardown();
            }
        })
    }
}

/**
 * Works with components based on LitElement
 * @param renderer the renderer function
 * @param BaseElement - LitElement or its subclass
 * @param options ...
 * @returns {Element}
 */
export function component(renderer, BaseElement = LitElement, options = {useShadowDOM: true}) {
    class Element extends BaseElement {
        static get properties() {
            return renderer.observedAttributes || [];
        }

        createRenderRoot() {
            return options.useShadowDOM ? this.attachShadow({mode: "open"}) : this;
        }

        constructor() {
            super();
            this._container = new LitContainer(renderer, this);
        }

        render() {
            return this._container.render()
        }

        updated(_changedProperties) {
            if (this._container[effectsSymbol]) {
                this._container.runEffects(effectsSymbol);
            }
            super.updated(_changedProperties);
        }

        disconnectedCallback() {
            this._container.teardown();
            super.disconnectedCallback();
        }
    }

    return Element
}

It does work for me at the moment although I might miss something. I need to test it a bit more.

But the problem is that in order to use this piece of code I have to compile my own lit-element-haunted :).

So haven't you thought about adding support for lit-element or about adding some kind of plugin infrastructure so that it would be possible to add this kind of stuff in runtime without recompilation?

I am ready to help if it is only a time issue :)

Thanks a lot!

matthewp commented 5 years ago

Thanks @jdin ! It's cool what you're doing. From the class perspective, I think instead of modifying the class Haunted creates you can simply extend and override the parts you need. For example (pseudo-code):

const BaseElement = component(render, LitElement);

    class Element extends BaseElement {
        static get properties() {
            return renderer.observedAttributes || [];
        }

        constructor() {
            super();
            this._container = new LitContainer(renderer, this);
        }

        render() {
            return this._container.render()
        }

        updated(_changedProperties) {
            if (this._container[effectsSymbol]) {
                this._container.runEffects(effectsSymbol);
            }
            super.updated(_changedProperties);
        }

        disconnectedCallback() {
            this._container.teardown();
            super.disconnectedCallback();
        }
    }

Basically you are just providing the hooks that LitElement expects and forwarding things that Haunted expects. Am I right so far? If so then I think it's a matter of formalizing the "Container" class. I'll have to think more about this. Let me know if I'm in the right direction though?

With this you could then write a function that takes a renderer that calls component() and then extends for a really not haunted-lit-element sort of api πŸ˜„

matthewp commented 5 years ago

Also, if you happen to wrap this up in another package I'd be happy to link from the readme here.

jdin commented 5 years ago

Hi @matthewp

The biggest problem that I have faced is that LitContainer has to use internal Haunted functions that are not available from outside. And it makes it impossible to extend Haunted without recompilation.

As soon as I have time I will push my changes to a fork, so you will see the full picture....

jdin commented 5 years ago

Hi @matthewp

I have just pushed my changes to this fork (see lit-element branch). So you can check it. The changes are mostly:

To make the life easier I have also made a pull request

As you can see it would not be possible to do it without recompilation as here and here I do import of some 'not exposed in a final build' functions which are used then in LitContainer.

I hope everything should be clear now :)

So what would be your suggestion:

matthewp commented 5 years ago

I don't want to implement LitComponent in Haunted. I've looked at your code but do not completely understand yet why a new container is needed. I'll look more into that and get back with you. I am excited by this possibility! πŸ˜ƒ

matthewp commented 5 years ago

@jdin Can you explain the use case a bit more? What does combining Haunted and LitElement give you? Do you have a LitElement base class with some functionality you want to have with functional components? Can you possibly provide a little example of how the usage of this feature would look like?

jdin commented 5 years ago

Hi, @matthewp

Thank you for your interest and your will to help!

We use LitElement and some customizable extensions based on LitElement. What I want is, indeed, to be able to use React Hooks API and functional components but based on LitElement not only HtmlElement as it is in Haunted at the moment.

Something like this:

const SomeComponent = ({myAttribute}) => html`Attribute = ${myAttribute}`; // with ability to use hooks here
customeElements.define('some-component', component(SomeComponent, LitElement)); // here can be checked if second argument is a subclass of LitElement then use LitElement renderer; and otherwise use Haunted renderer

By using new LitContainer I am using our enhanced LitElement renderer functionality and not the one in core.js in 'Haunted'.

Let me know if it is clear now or if I am missing something :)

matthewp commented 5 years ago

@jdin Thanks, I'd like a little more on the "customizable extensions based on LitElement" part. From your example it doesn't seem like this some-component element is really taking advantage of any of the features LitElement provides. Or is it? Which features are you getting out of LitElement in this example?

I ask these to help better come up with a good API for this, not that I doubt there's value in this feature.

jdin commented 5 years ago

@matthewp There are some class mixins built on top of LitElement (similar to this one, but we have much more!). And it would be really nice if we can use those class mixins with functional approach on the top.

matthewp commented 5 years ago

@jdin Thanks, can you give a small example of how that mixin is used (I couldn't find one). What I'm wanting to do is to create a little demo of using LitElement and its features. With that demo it will make it easier to understand how Haunted can fit in with it.

This may sound like a unnecessary process but I think it's always better to really understand the use-case of a feature before starting to implement. Thanks for you patience :-)

jdin commented 5 years ago

@matthewp Have a look here in storybook. Localization probably is a good example of a mixin which is used overall. Let me know if it is clear or you need more information!

jdin commented 5 years ago

It would be so nice if we could use LocalizeMixin smth like:

const MyComponent = () => ...
MyComponent.styles = [...]
MyComponent.localizeNamespaces = [...]
customElements.define('my-localized-component', component(MyComponent, LocalizeMixin(LionLitElement))
jdin commented 5 years ago

Hi @matthewp. I am following the discussions you have with @chase-moskal and @jpray and waiting for this awesome pull request . This pull request does almost what we need but not everything. It does not yet cover LitElement support completely. For LitElement it would be needed possibility to override a bit more within Container and Component. Do you still plan to add LitElement support?

But in general I feel that the project goes in right direction! Keep it up!

matthewp commented 5 years ago

@jdin I'm still thinking about that. After that PR lands I can probably put some more thought into this issue. Sorry for the wait.

matthewp commented 5 years ago

Now that 4.5.0 is out I'm going to look into this soonish.

matthewp commented 5 years ago

I've been thinking more about this. I think this is likely the next thing I work on. So what makes this tricky is that Haunted has 2 things that are contained with the Container type:

lit-element does scheduling differently, render/commit happen synchronously. We might go that route too, as render()ing should be pure and not cause DOM read/writes. So I think just having 2 phases will be ok (and probably clear up some other bugs people have).

But nevertheless, if you are using lit-element you probably don't need Haunted's scheduler at all. The one thing you might need it for is running effects.

I wonder if it makes sense to split these things into 2 different classes. State and Runtime. It might look something like this:

let state = new State(() => {
  // state changed. Call into the schedule to update.
  // In lit-element I think this is forceUpdate();
});

Runtime would be Haunted's runtime scheduler. It would use a State and call into the update cycle used by component and virtual.

I'm going to experiment with this, but let me know if this makes any sense from your perspective @jdin πŸ˜€

jdin commented 5 years ago

Hi @matthewp Indeed LitElement has its own lifecycle and own scheduler so it is not really needed to use haunted scheduler. Actually in my first quick implementation of LitContainer I did not use haunted scheduler at all and the hooks where executed on updated stage (see first comment):

        updated(_changedProperties) {
            if (this._container[effectsSymbol]) {
                this._container.runEffects(effectsSymbol);
            }
            super.updated(_changedProperties);
        }

I will be happy if at the end LitElement's styles and attribute-property conversion will work so that I can write smth like this:

const MyComponent = () => ...
MyComponent.styles = [...]
MyComponent.properties = {...}
customElements.define('my-component', component(MyComponent, SomeMixin(SomeLitElementExtension))

If I have understood you correctly the code above should work with your changes.

I am looking forward for your new merge request!

JosefJezek commented 5 years ago

@matthewp do you have any solution?

matthewp commented 5 years ago

Not quite yet. I have worked on this some but have some more to do. Will likely reply here when I have more info. This will likely be the next Haunted feature.

matthewp commented 5 years ago

I have a branch with this working here.

A basic example looks like:

import { LitElement } from 'https://unpkg.com/lit-element@2.2.0/lit-element.js?module';
import { State } from 'haunted';

export default function(renderer) {
  return class extends LitElement {
    static get properties() {
      return renderer.observedAttributes || [];
    }

    createRenderRoot() {
      return this.attachShadow({mode: "open"});
    }

    constructor() {
      super();
      this.haunted = new State(renderer, () => this.update());
    }

    render() {
      return this.haunted.render();
    }

    updated(_changedProperties) {
      this.haunted.runEffects();
      super.updated(_changedProperties);
    }

    disconnectedCallback() {
      this.haunted.teardown();
      super.disconnectedCallback();
    }
  }
}

I'd really like some feedback from lit-element users. I think eventually this should be released as a prerelease but I'm still not sure about the API names. What does everyone think? (I also considered calling the constructor Haunted instead, but is that confusing with the new lower-case haunted export?)

Also, when this makes its way into a Haunted release, would anyone be willing to create a haunted-lit-element or something like that which provides a mixin for this base class?

jdin commented 5 years ago

Hey @matthewp Looks cool for now! I have to try it out myself and I let you know if there are comments. As about Haunted vs haunted - I do not mind. Both perfect for me :). As about haunted-lit-element, indeed it is a good idea to have it present. I would also add support for LitElement's styles there. I will try to make it but I would need review help from some of you to make sure that I am not missing something.

matthewp commented 5 years ago

Ideal API:

import mixinHaunted from 'haunted/mixin';
import { Element } from 'lit-element';

const LitHauntedElement = mixinHaunted(Element);

class HelloWorld extends LitHauntedElement {
  render() {
    const [count, setCount] = useState(0);

    return html`Count: ${count}`;
  }
}

And here's one with SkateJS:

import Element, { h } from '@skatejs/element-preact';
import mixinHaunted from 'haunted/mixin';

class ElementPreact extends mixinHaunted(Element) {
  static props = {
    name: String
  };
  name = 'World';
  render({ name }) {
    const [currentName, setName] = useState(name);

    return <span>Hello, {currentName}!</span>;
  }
}

customElements.define('element-preact', ElementPreact);

This is a bit better, I think? This could work with any base class that uses render as a lifecycle method, which is a lot of base classes.

This shows haunted/mixin but this could likely be in core, I think it's just a refactor and wouldn't necessitate a file size increase.

Do people like this better than what was posted above? cc @jdin @treshugart @justinfagnani @JosefJezek

benjamind commented 5 years ago

This proposed API is I think is exactly what we want for using hooks with lit-element. Gains all the advantages of the simplicity of the class based elements but also the elegance of hooks when needed. Nice work @matthewp.

jdin commented 5 years ago

Hi @matthewp Maybe I am the only one here so please do not shoot me but I like the previous approach more. 😌 To be honest I wanted to get away from classes and to work only with pure functions. But with the last mixin approach you have got state in a class and state in a hook which seems very strange to me. And how do you handle the lifecycle? Are you planning to use again haunted scheduler? How do you know when to call runEffects? Do you have an implementation of mixin already?

matthewp commented 5 years ago

@jdin Ah yeah, effects make this hard. This is a bit hard to come up with the right level of abstraction. An element mixin has to know about more than just render, so you can't really do that in a generic way.

Maybe it will be easier to come up with an API if we first break down what a hooks container needs. It needs:

  1. A place to store its state.
  2. The ability to call the render function (because it needs to tell hooks that its the current instance).
  3. The ability to call effects (which happen some time after render).
  4. The ability to call back out to the "controlling" class to tell it to update when hook state changes.

So given those requirements, maybe the first method is the best approach. Maybe the API is still kind of weird. What about something like:

class Child extends LitElement {
  constructor() {
    super();

    this.haunted = new Container({
      render: () =>  {
         return this.render();
      },
      update: () => {
        return this.update();
      }
    });
  }

  render() {
    const [count, setCount] = useState(0);

    return html`Count: ${count}`;
  }
}

This is a litter nicer than the first API, and gives implementers the freedom to build mixins like LitHauntedElement that are class based, or you could build one that is function base and works like:

const App = () => {
  return html`Hello world`;
}

// Implement LitElement stuff here.
class AppElement extends litHaunted(App) {
  props = { ... }
}

customElements.define('my-app', AppElement);
benjamind commented 5 years ago

This approach also works quite nicely yes, and agreed on being explicit about creating a container for the hooks 'environment' to run in as it delineates the two worlds nicely.

justinfagnani commented 5 years ago

In LitElement's case, render() is not the right place to hook into. Overriding update() should be all you need to do to scope synchronous work - render() is just a step inside the update phase. And Haunted should never call update() or render() directly, but delegate to UpdatingElement's requestUpdate() method.

justinfagnani commented 5 years ago

@benjamind is this really going to help with your hooks use cases? Hooks seem pretty tightly tied to their container, so you wouldn't be able to share them across other non-Haunted containers.

benjamind commented 5 years ago

@justinfagnani after our discussion the other day, we've been investigating the hooks that we were hoping to reuse and sadly at this stage it does not look like they would be very easily reused without more further abstraction being done.

So this is no longer a massive driver for us at the moment, but I'm keeping an eye on it with an eye for potential reuse opportunities. But at this time I've seen very little benefit from hooks across components, they just don't tend to get the re-use benefits that are being promoted and where they are getting re-use they just make debugging hard.

matthewp commented 5 years ago

@justinfagnani Haunted has to set up some globals that make hooks work before and after render() is called. lit-element doesn't have a beforeRender and afterRender does it? The above snippet is not exactly correct, but I think a LitHauntedElement would have to override the user-provided render() in order to do that bookkeeping work.

Might be able to simplify this to something more like:

let container = new Container(() => {
  // Callback is called when state updates
  this.requestUpdate();
});

// Some time later
container.beforeRender();
// Do rendering however
container.afterRender();

Maybe a little lower level this way πŸ€·β€β™‚οΈ

justinfagnani commented 5 years ago

@matthewp UpdatingElement has performUpdate() and update() as overridable points for this.

I think the simplest thing you could do is:

const hauntedContainer = Symbol();
export const Haunted = (Base) => class extends Base {

  constructor(...args) {
    super(...args);
    this[hauntedContainer] = new Container(() => this.requestUpdate());
  }

  update(changedProperties) {
    this[hauntedContainer].beforeRender();
    super.update(changedProperties);
    this[hauntedContainer].afterRender();
  }
}
export const HauntedLitElement = Haunted(LitElement);
export const HauntedUpdatingElement = Haunted(UpdatingElement);

If you wanted to support hooks in updated() and firstUpdated() (is there a use-case for that?), then you'd want to override performUpdate() which is a bit more complicated due to error handling.

matthewp commented 5 years ago

Thanks @justinfagnani, super helpful. So I have an updated implementation that can be used like this:

import { LitElement } from 'https://unpkg.com/lit-element@2.2.0/lit-element.js?module';
import { State } from '../../haunted.js';

export default class LitHauntedElement extends LitElement {
  constructor() {
    super();

    this.haunted = new State(() => this.requestUpdate());
  }

  update(changedProperties) {
    this.haunted.beforeRender();
    super.update(changedProperties);
    this.haunted.afterRender();
  }
}

export const litHaunted = (renderer) => {
  return class extends LitHauntedElement {
    render() {
      return renderer.call(this, this);
    }
  }
};

I like how extremely simplistic this is. And it can be used as a class style or a function style. Either:

class MyElement extends LitHauntedElement {
  render() {
    const [ count, setCount ] = useState(0);
    return html`...`
  }
}

Or:

const App = litHaunted(el => {
  const [ count, setCount ] = useState(0);
  return html`...`
});

I'm going to create another example that uses SkateJS, and if that works out I'll probably do an alpha so those interested in creating these integrations can play around with it and provide more real world feedback.

LarsDenBakker commented 5 years ago

That looks really interesting!

matthewp commented 5 years ago

I added an example with SkateJS in the lit branch. It looks like this:

import { withComponent } from 'skatejs';
import  withPreact from '@skatejs/renderer-preact';
import { State } from 'haunted';

const Base = withComponent(withPreact());

export default class extends Base {
  constructor() {
    super();
    this.haunted = new State(() => this.triggerUpdate());
  }

  renderer(...args) {
    this.haunted.run(() => super.renderer(...args));
  }
}

I got rid of beforeRender() and afterRender() and instead made it be .run(fn) that just lets you run a function in the context of the State.

At this point I think the next steps would be for me to create a PR with the new API laid out, and probably do a prerelease so people can try out and see how it feels. I'll include some example integrations as gists.

jdin commented 5 years ago

Thanks a lot @matthewp and @justinfagnani! It looks now really what we wanted. Can't wait for next release! :ghost:

matthewp commented 5 years ago

The beta is out! See the release notes here: https://github.com/matthewp/haunted/releases/tag/v4.6.0-beta.0

Please keep me in the loop on integrations as you test this out. Anything that works or doesn't work let me know and we'll see about refining the API. Want to give it a little time to marinate and if everyone's happy we can then release.

Thanks for pushing me to do this issue, I think it's making Haunted much better!

treshugart commented 5 years ago

This looks rad, @matthewp. Sorry for the late reply!

jdin commented 5 years ago

Hi guys, I have made my first attempt to build the missing connection between lit-element and haunted here (and my first attempt to publish something to npm). To be honest it is almost just a copy from the examples in haunted but to my mind it is good to have it still available as a package. I am open for comments and critics. @matthewp didn't you think to have a scope @haunted? It would probably be useful to publish all the connection libraries with this scope. For example: @haunted/lit-element-support, @haunted/skatejs-support, etc.

matthewp commented 5 years ago

@jdin I'm happy to have a scoped package if it would be helpful to people, but I don't want to "own" all of the packages therein. So I guess it can be a community thing where we're all owners.

matthewp commented 5 years ago

Looks great btw! Hoping to release 4.6 within the next week or so!

Gladear commented 5 years ago

Having scoped packages would also "solve" issue #80. There could be an @haunted/core package that would have no dependency, which would make users free to choose the rendering library they want to use, without being forced to download lit-html.

matthewp commented 5 years ago

haunted/core does not include lit-html.

matthewp commented 5 years ago

This is the kind of thing that makes me leery about scoped packages and breaking up haunted: https://twitter.com/geddski/status/1174760156590039040

chase-moskal commented 5 years ago

leery about scoped packages and breaking up haunted:

i agree

many folks are overzealous in breaking a project up into microfragments, like with koa -- practically every function is its own repo

i think i'd be favor of keeping several entrypoints, for interoperability with different libraries, within the haunted repo, eg, haunted-lit-html.js, haunted-lighterhtml.js, haunted-htm.js

Gladear commented 5 years ago

To me, the problem with multiple entrypoints is handling dependencies. Currently, Haunted depends on lit-html, which users of other rendering libraries don't care about. If other entrypoints are created, Haunted would depend on other libraries, which I don't think is a good idea. Having multiple packages would allow each package to have different dependencies, depending on which are required. Maybe another solution would be to remove all dependencies (certainly keeping them as devDependencies) and let the users handle them ?

matthewp commented 5 years ago

Let's refocus the conversation surrounding the idea of having scoped packages for integrations. We can relitigate breaking up Haunted in another issue.

The downside to having a scoped registry for community packages is, unless I'm wrong on this, I think anyone with publish rights on @haunted/foo would also have publish on @haunted/bar. Since @jdin would be owner on @haunted/lit-element then I think they would not want someone else to have the ability to publish versions of that package.

chase-moskal commented 5 years ago

@Gladear

If other entrypoints are created, Haunted would depend on other libraries, which I don't think is a good idea.

i believe there's a different way of looking at it

haunted can list lit-html, lighterhtml, htm, etc, as dependencies in its package.json β€” but remember that doesn't mean those dependencies will actually be loaded at runtime by a consumer of haunted β€” it really only means that those dependencies will be available in node_modules to the consumers own bundling tooling

the dependencies loaded at runtime are based on the entrypoint chosen by the consumer, and its tree of imports β€” such that a consumer asking for haunted/dist/haunted-lighterhtml.js won't load lit-html

however, yes, the dependencies loaded in the package.json will increase the consumer's available node_modules, but i don't think having extra modules available is something that anybody's worried about, and is actually quite convenient

@matthewp

Let's refocus the conversation surrounding the idea of having scoped packages for integrations. We can relitigate breaking up Haunted in another issue.

this confuses me.. doesn't scoped packages for integrations imply breaking up haunted into multiple repositories or packages?

my vote is in favor of the haunted repository containing everything officially supported by haunted: its core, auxiliary functionality, and integrations with many rendering options such as lit/lighter/htm β€” a single repository for haunted keeps thing simple, avoids the issue you mention about publishing rights, consolidates issues and pull requests to one convenient location, includes everyone interested in haunted in the same discussions, and makes maintenance generally easier β€” and at no real cost to performance (i think i'm seeing here that the perception of a performance cost is the main reason split things up)

i think koa is the worse example of micro-packaging gone haywire, and represents a bad developer experience to be avoided

:wave: chase

matthewp commented 5 years ago

this confuses me.. doesn't scoped packages for integrations imply breaking up haunted into multiple repositories or packages?

No, it just means special integrations like @jdin's haunted-lit-element could be @haunted/lit-element instead. It wouldn't change haunted in any way.

jdin commented 4 years ago

Just noticed that this one is still open. Closing it as this one: https://github.com/jdin/haunted-lit-element seems to be working ...