feat: add support for adding dom element properties

[ntilwalli]

This feature is an attempt to offer the same dom props functionality offered by VNodes in @cycle/dom. I've chosen to name the special prop domProps but that could obviously be changed. I would find this feature useful but dunno if it's more appropriate as a fork. I use this feature to sometimes tag elements with lambdas that I want applied in some reducers. As it currently stands (if my understanding is correct) only attributes are supported which limits element-specific data to strings and numbers only.

Usage:

const sym = Symbol()
const print$ = souces.react.select(sym).events('click')
   .map((ev) => state => { 
      return ev.target.reducer(state) 
   })
const react$ = of(button(sym, { domProps: { reducer: (state) => ({ ...state, foo: 3 }) } }))

Not necessarily expecting a merge, just looking for simple feedback for now.

This PR follows #10

[staltz]

Hey @ntilwalli, I'll make some questions just to understand your use case: since this domProps is implemented via React refs, why not use refs directly in user land? Maybe the hyperscript doesn't support it, but you can also create conventional React components (with the class syntax or hooks syntax) which use refs and then communicate with the rest of your app via props and onFoo events.

[ntilwalli]

@staltz, thanks for the response. I'm not clear on the question. Are you wondering why this domProps work merits being a part of the driver when I could directly apply it myself for any relevant element in user-land, via a direct ref + componentDidUpdate application for each relevant element? Or are you asking why replicating @cycle/dom/snabbdom's props functionality is useful given to available tools within the React ecosystem?

My use-case is wanting to set custom attributes on my input elements to have lambda values, as far as I can tell that's not possible (only strings and numbers are valid attribute values). I use this functionality for my custom forms implementation. By attaching reducer lambdas directly to input elements I can have a common intent function that does not vary for each form (which enables me to have configuration-driven form validation). Each element includes a commonly agreed-upon property (containing a lambda) which I extract during intent which then gets applied (along with it's changed value) in the reducer.

Basically during intent I want to access a value on the element that is not encodable in an attribute or data-attribute but is easily encodable as a property.

Having domProps will make transitioning from @cycle/dom to react-dom easier for me.

Did I answer your question?

[staltz]

My use-case is wanting to set custom attributes on my input elements to have lambda values, as far as I can tell that's not possible (only strings and numbers are valid attribute values). I use this functionality for my custom forms implementation.

I see now, thanks. Because we're using React (and its huge ecosystem), I'm keen to see what can we do on the React side before we look into implementing this into cycle/react(-dom). Like you said, this was implemented in snabbdom. I think both snabbdom and React are responsible for Virtual DOM diff/patch and hooks (thus, interpretation of VNodes into DOM side effects) while cycle/dom and cycle/react(-dom) are responsible for providing a Stream interface and particularly DOM event streams produced via DOMSource.select(x).events(y). I think separating those responsibilities helps to know where the change should be made when you want a change.

That said, I couldn't find anything about domProps nor elProps in cycle/dom, neither in snabbdom. Could you point me to the implementation of those things somewhere?

[ntilwalli]

The term domProps was used to indicate that the properties will be applied to the dom element created by this virtual dom node. In snabbdom you can do: input('.some-class', { attrs: {type: 'text', placeholder: 'Type test here...' }, props: { foo: () => 'bar' }). Notice the props property. That property indicates the properties that should directly be created/updated on the DOM element itself. I'm simply trying to replicate that functionality here.

It would ideally be added to the functionality of react itself which would allow for a clean separation of concerns similar to @cycle/dom and snabbdom. But changing react to be more symmetric to snabbdom in this way felt like such a reach, so I added it here. I personally think the props functionality in snabbdom is super-useful so I don't want to sacrifice it to use react in cycle. That being said, I may be the only person who cares about this. I'm now inclined to think of this meriting a fork instead of a merge. If you agree, I'll go ahead and close this.

The reference to elProps in code snippet above was a typo (I've updated it), domProps is the term I'll use.

[staltz]

I see, you want parity between snabbdom and cycle/react hyperscript as much as possible, to facilitate migration. I think there's something decent that can be created here, which isn't quite a fork of cycle/react. There could be a library, perhaps cycle-react-hyperscript-snabbdom (feel free to choose a better name of course) that forks only the hyperscript part of this repo, and aims at providing an API that is very close to snabbdom's API, including props but (why not) also style module, hooks, etc. That would be a useful abstraction over React. This library could use (i.e. import) the Incorporator from this repo.

[ntilwalli]

Yeah, that's a good idea, but you seem to imply that it can be accomplished by a h.ts fork only. I'm quite new to React, so I may be missing some React basics, but aren't changes in incorporate.ts and incorporator.ts required as well?

How can snabbdom's props functionality be replicated without access to the underlying element? How can I access that element without adding a possibly missing ref in Incorporate? How can I do that work and/or integrate snabbdom-style hooks without plugging into the lifecycle events in Incorporator?

[staltz]

Yeah, honestly figuring out the "tricks" that were needed in cycle/react were not obvious at all.

I could be wrong, but I believe in your case the implementation is possible as a composition of React components. E.g. incorporate.ts is a higher-order component (HOC), but when you're using the hyperscript (h.ts), you're oblivious to whether the underlying component is button or incorporate(button), this is transparent to the user. I think you could add refs to a HOC (for the snabbdom-like abstraction for React) and it could wrap the underlying Incorporator transparently. Maybe we could use forwardRef twice?

I think one good way to go about this is modifying this PR until we figure out what parts can be put in a separate library. I'm curious myself to try this out, good that you have tests for it already.

[staltz]

I got this far:

I created file h2.ts forking from h.ts.

export function domPropify(Comp: any): ComponentType<any> {
  class DomProps extends Component<any, any> {
    private ref: any;
    private domProps: any;
    constructor(props) {
      super(props);
      this.domProps = this.props.domProps;
      this.ref = createRef();
    }

    public componentDidMount() {
      if (this.domProps && this.ref) {
        Object.entries(this.domProps).forEach(([key, val]) => {
          this.ref.current[key] = val;
        });
      }
    }

    render() {
      const p: any = {ref: this.ref, ...this.props};
      delete p.domProps;
      return createElement(Comp, p);
    }
  }

  return DomProps;
}

// ...

function hyperscriptProps<P = any>(
  type: ElementType<P> | keyof ReactHTML,
  props: PropsLike<P>,
): ReactElement<P> {
  if (!props.sel) {
    return createElement(type, props);
  } else {
    return createElement(domPropify(incorporate(type)), props);
  }
}

The tests are a bit "broken" because there is actually no ev.target in this case, because we're testing against a Touchable, which is our own dummy class. As far as I know, there is nothing hooked up to the DOM in these tests, so getting something looking like ev.target will be challenging...

This is the test (broken because I'm using ev.target):

  it.only('applies domProps to element when given', (done) => {
    function main(sources: {react: ReactSource}) {
      const inc$ = sources.react.select('button').events('press');
      const count$ = inc$.fold((acc: number, ev: any) => acc + ev.target.blah, 0);
      const vdom$ = count$.map((i: number) =>
        h2(Touchable, {sel: 'button'}, [h2('div', [h2('h1', {}, '' + i)])]),
      );
      return {react: vdom$};
    }

    let turn = 0;
    const RootComponent = makeCycleReactComponent(() => {
      const source = new ReactSource();
      const sink = main({react: source}).react;
      return {source, sink};
    });
    const r = renderer.create(createElement(RootComponent));
    const root = r.root;
    const check = () => {
      const to = root.findByType(Touchable);
      const view = to.props.children;
      const text = view.props.children;
      assert.strictEqual(text.props.children, `${turn * 3}`);
      to.instance.press();
      turn++;
      if (turn === 3) {
        done();
      }
    };
    setTimeout(check, 50);
    setTimeout(check, 100);
    setTimeout(check, 150);
  });

I'll go to sleep now but I thought this might be useful to you.

[ntilwalli]

Dunno if cypress tests are appropriate but I added one (can remove if there's a leaner way). As of now that test is really wonky. I had to use a then and access the underlying element and do .then(($el) => { cy.wrap($el.foo).should('eq', 3) }). The ideal way (which is not working for some reason) would be .its('foo').should('eq', 3). But the hack allowed the test to pass for now as we work through...

[ntilwalli]

I integrated your domPropify code and added a similar domHookify HOC as well. My understanding is:

• style and key should work out of the box, sincesnabbdom and react are symmetric in the way the treat those props
• need to test hooks
• could change property names from domProps to props and domHooks to hooks to be symmetric.
• need to parse selectors given in snabbdom hyperscript to extract classes, and ids and apply them into className and id respectively.
• could integrate with thunks

Is anything missing?

[staltz]

Is anything missing?

You could consider implementing all those HOC functions as just one function. It might not be a big problem for performance, but it might still affect performance.

And/or you could consider allowing third-party HOCs much like snabbdom allows third-party "modules". Maybe someone has a codebase with custom snabbdom modules that they'd like to support an alternative to when migrating to cycle/react.

Just to make sure though, you intend to make a separate library for h2.ts? Like I mentioned:

I think one good way to go about this is modifying this PR until we figure out what parts can be put in a separate library.

[ntilwalli]

I would have no issue making this a separate package, but given the updated modules orientation around the latest implementation (and it's seamlessness with the current approach), I wonder, why not just merge directly? There would be a very minor perf hit due to the testing of modules enablement on each render, but prob inconsequential since it's O(1). Take a look at examples/index.js for how it is used. The big code update is in Incorporator. Since React doesn't give access to an init function like snabbdom (to indicate which modules should be enabled) I used file-level global vars to store the module configuration.

• Enabling modules requires calling setModules which currently sits in Incorporator
• The module hooks are componentDidMount, componentDidUpdate, and componentWillUnmount.
• The property name associated with each module is the same as the prop-name expected in the props which is applied by that module.
• The example implements domProps and domClass modules.

Suggestions/critiques are welcome.

[staltz]

Hey @ntilwalli, your new solution is overall smaller and slicker, but I'm hesitant to do deep changes in Incorporator, and would still prefer an external library. My reasoning is not performance, but separation of concerns and composability. Libraries are cheap to make, and it's good when they compose, because then you don't get feature creep in any one of them, and they focus on solving one problem each. E.g. that's why I made @cycle/react-native separate from @cycle/react, and cycle-native-navigation separate from @cycle/react-native, as well as all React Native related drivers in their own libraries. In React world, HOCs are the best so far for composability.

It would be best if Cycle React could do only what it proposed to do in the readme:

Interoperability layer between Cycle.js and React

• React (DOM or Native) as the rendering library in a Cycle.js app
• Convert a Cycle.js app into a React component
• Support model-view-intent architecture with isolation scopes

A "Snabbdom-like abstraction for cycle/react" is a departure from the features listed above, deserving its own module.

[ntilwalli]

@staltz I've integrated your feedback. Modulizer is a new module/class that layers its functionality on top of Incorporator. Modules enablement happens through the new useModules function, which (when activated) swaps out 1) the Incorporator class for the Modulizer class for use when incorporate is called and 2) the function which tests whether to apply incorporation at all. The integration happens in incorporate because I wanted to take advantage of the element caching that happens there. Let me know your thoughts.