jorgebucaran / superfine

Absolutely minimal view layer for building web interfaces
https://git.io/super
MIT License
1.57k stars 78 forks source link

Support for React-like `ref`s? #184

Open mcjazzyfunky opened 3 years ago

mcjazzyfunky commented 3 years ago

Wouldn't it be a nice and useful feature if superfine supported ref pseudo properties like React, Preact, Dyo, uhtml etc. do?

const elemRef = { current: null }
// [...]
const content = h('some-element', { ref: elemRef })

Here's a little demo (based on a R&D web-component toy library that uses a ref-supporting patched version of superfine internally): TypeScript version JavaScript version

FYI: Here's a naive quick'n'dirty implementation which obviously only provides part of the requested functionality: LINK

jorgebucaran commented 3 years ago

What's the fully requested functionality?

mcjazzyfunky commented 3 years ago

[Edit: fixed the URLs for the demos several times]

What's the fully requested functionality?

I basically meant that:

For the latter please check the following React demo (everything is working fine there ... Preact and Dyo are working accordingly): https://codesandbox.io/s/ref-demo-b0kmw?file=/src/index.js ... and compare it with that naive superfine ref support implementation that I have mentioned above: https://codesandbox.io/s/ref-demo-forked-wirto?file=/src/index.jsx

By the way: This is also not working with the unmodified uhtml library: https://codesandbox.io/s/ref-demo-r25v2?file=/src/index.js

mcjazzyfunky commented 3 years ago

Sorry, I have mixed up some of the URLs in my previous comment - have fixed it.

jorgebucaran commented 3 years ago

We don't really have to support the exact API as React, Preact, since Superfine does not seek compatibility with any of them, but if that's all it takes or even a little more, I'd definitely consider adding this to the project.

Do you want to work on a PR?

mcjazzyfunky commented 3 years ago

Sure, I can provide a PR if you like. I guess the changes in superfine will look something like the following (please ignore the comments on top of the file): https://github.com/js-works/js-element/commit/a7767704a3d1ac1ba5a09d707a55258fc9fbb973

This is working for my small demo, but I have to check some edge cases before I can prepare the PR for superfine. I do not know the exact coding guidelines for the superfine project but I guess for the main/index.js file the credo is "shorter is better" (both in LOC and built distribution package) - please let me know if I am wrong.

jorgebucaran commented 3 years ago

Don't worry too much about code style or golfing the code too much at this point. We'll get to that after it works. 💯

mcjazzyfunky commented 3 years ago

@jorgebucaran Could you please tell me, what shall be done with the unit test for this change? Do you want to continue to use twist as test library? Seems like twist is not working properly on Windows ("Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only file and data URLs are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'").

My initial version of the superfine tests (in one of my projects) is using Jest but I can switch to whatever test library you like: https://github.com/js-works/js-element/blob/master/src/test/superfine.test.ts

The tests are really important, I would have done an ugly mistake if I didn't implement the test suite for that ref feature. By the way, here's the current implementation which seems to work (please ignore this "oldVKiss" typo this has already been fixed): https://github.com/js-works/js-element/commit/480db0c93174da3a8256c02ae7a20eaea3c38cd0

jorgebucaran commented 3 years ago

I'm not switching test libraries.

On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'").

Yeah, I don't have a Windows box, so I haven't been able to test Twist on it. I want to fix it, though.

jorgebucaran commented 3 years ago

Do you mind ELI5 what refs are for? How are they different from lifecycle events oncreate, onupdate, and onremove which Superfine used to support way back (but not anymore)?

mcjazzyfunky commented 3 years ago

Let's assume you have a fancy component library which is based on superfine. Now let's assume you want to implement a component VideoPlayer. This VideoPlayer shall have the usual control buttons video players normally have, like Play, Pause etc. Somewhere in your VDOM tree you have to use a h('video', ...) virtual node. But how do the event handlers for those Play and Pause buttons work? They need the actual HTMLVideoElement instance to call the play() and pause() methods on it. But how do you get this HTMLVideoElement instance? One way would be to use DOM's querySelector() method, but then you would normally need the HTMLElement of the VideoPlayer instance itself. That's of course possible, but using React-like ref objects and ref callbacks is a much nicer, cleaner and more flexible way to do so.

How are they different from lifecycle events oncreate, onupdate, and onremove which Superfine used to support way back (but not anymore)?

If I understand that correctly it may have been possible in the old days of superfine to write a function ref which could be used as follows:
h('video', { ...ref(videoRef), src: videoSrc }) or (with JSX) <video {...ref(videoRef)} src={videoSrc}/>

with

function ref(refObjectOrCallback) {
  return {
    oncreate: ...,
    onupdate: ...,
    onremove: ...
  }
}

This may work the same way as React-like refs do. Nevertheless the <video ref={videoRef} /> syntax is nicer and those lifecycle properties have been removed in superfine for a reason, I guess.

jorgebucaran commented 3 years ago

Do you have an example using ref with your patched version of Superfine to animate removing an element from the DOM?

whaaaley commented 3 years ago

I agree this feature would be nice to have. I faced a similar issue just yesterday when trying to making an accessible drop down menu. I was trying to call focusin and focusout when certain conditions were met. My solution was not elegant at all.

If I understand correctly the proposed API is this? Where ref returns a function, that, when passed as a prop in your view, is called by superfine while patching, which returns the element.

import { h, text, patch, ref } from 'superfine'

const Component = () => {
  const parentRef = ref()

  return h('div', { ref: parentRef }, [
    h('div', {
      onclick: () => {
        parentRef.remove() // or something
      }
    }, [
      text('remove parent')
    ]),
    text('parent')
  ])
}
mcjazzyfunky commented 3 years ago

Do you have an example using ref with your patched version of Superfine to animate removing an element from the DOM?

@jorgebucaran Sorry, I completely forgot to answer. You mean something like @whaaaley's example? But why exactly is the animation important? Let me know and I will prepare a little demo.

@whaaaley It should be parentRef.current.remove(), but the rest is okay (in React your ref function is called createRef, but I personally have no preference)

[Edit] @whaaaley Actually a ref can be both a function or an object { current: ... }

type Ref<T> = { current: T | null } | ((ref: T | null) => void)
jorgebucaran commented 3 years ago

The animation is important because it's one of the common use cases for lifecycle events, which this feature resembles.

whaaaley commented 3 years ago

@mcjazzyfunky That's interesting. Is there an advantage to using the object? Maybe it avoids the import? I feel like using a ref function may be more fitting for Superfine, but that's just a feeling I have. Tucking away the (potential?) mutation that will happen inside of Superfine rather than exposing the object to be accessed, pre-mutation. (But don't mind me I don't know much about it)

Also I remember animations with lifecycle events was a hot issue for while in Hyperapp. This seems like a decent solution to me. I do like it more than the old lifecycle events at least.

mcjazzyfunky commented 3 years ago

@whaaaley Actually in some cases a ref function suits better in other cases a ref object. Normally I use ref objects. Actually it's just one line of code to support both :wink: But there's something wrong with your demo. If parentRef is a function then it should be parentRef().remove() (given that parentRef is a read/write hybrid depending whether it's called with zero or one argument). Normally function refs are more like:

import { h, text, patch, ref } from 'superfine'

const Component = () => {
  let divElem = null

  return h('div', { ref: (elem) => { divElem = elem } }, [
    h('div', {
      onclick: () => {
        divElem.remove() // or something
      }
    }, [
      text('remove parent')
    ]),
    text('parent')
  ])
}
whaaaley commented 3 years ago

@mcjazzyfunky I see now. That's very similar to the old lifecycle events. (That's not to say I wouldn't benefit from them coming back in a limited capacity. It's always super awkward to call methods on elements.)

jorgebucaran commented 3 years ago

@mcjazzyfunky Sorry, I completely forgot to answer. You mean something like @whaaaley's example? But why exactly is the animation important? Let me know and I will prepare a little demo.

Animating the removal of a DOM element is challenging because Superfine has to defer removing the element after the animation and there's currently no way to do that without mutation observers or hiding the element instead of removing it (which is not too bad).

whaaaley commented 3 years ago

Animating the removal of a DOM element is challenging because Superfine has to defer removing the element after the animation and there's currently no way to do that without mutation observers or hiding the element instead of removing it (which is not too bad).

Sorry if I'm missing something obvious (I usually am), but what about onanimationend and ontransitionend element handlers and letting users implement something like this? https://codepen.io/dustindowell/pen/vYgbJXN. These handlers won't work on ie11, but support seems pretty good on everything else.

Edit: I think the main appeal of refs is just to avoid putting queries all over the place rather than try to hook into lifecycles. Doing a query feels incorrect when the node refs are right under our feet. Here's a demo I pulled out of one of my apps, a markdown text editor with synced scrollbars. Without refs a query would have to be run on every scroll event. https://codepen.io/dustindowell/pen/zYNePEp

mcjazzyfunky commented 3 years ago

Do you have an example using ref with your patched version of Superfine to animate removing an element from the DOM?

@jorgebucaran Not really sure whether this is want you've meant, but please see here: https://codesandbox.io/s/superfine-animation-sz9k6?file=/src/index.tsx

Please let me know when you need something more/else.

[Edit]: Actually this demo does not use refs ... where exactly do I need refs for animations?

jorgebucaran commented 3 years ago

What's the difference between refs and the old oncreate, onupdate, and onremove lifecycle events?

mcjazzyfunky commented 3 years ago

What's the difference between refs and the old oncreate, onupdate, and onremove lifecycle events?

A different, but out-of-the-box better useable API for basically the same thing. Refs may be a tiny little less powerful, as they do not care about onupdate events.

<video ref={videoRef}> is syntactically much nicer than <video {...ref(videoRef)}> and does not need an additional helper function.

And refs support both ref callback functions and ref objects.

What have been the use cases for oncreate, onupdate and onremove in previous Superfine versions and why have those lifecycle events been removed?

whaaaley commented 3 years ago

I JUST realized that we don't really need this feature because it already exists! Well maybe... If there's any caveats to doing it this way please let me know, but so far so good in my current apps.

import { h, text, patch } from 'superfine'

const view = () => {
  const ref = { vnode: null }

  const click = () => {
    console.log(ref.vnode.node)
  }

  return ref.vnode = h('div', { onclick: click }, [
    text('hello')
  ])
}

const node = document.getElementById('app')
patch(node, view)

Edit: This combined with a little once lock utility simulates onMounted lifecycle event. I have ideas on how to implement other events using the same idea but I'll cross that bridge when I need to.

export default () => {
  const storage = []
  let lock = false

  const handler = () => {
    lock = true

    for (let i = 0; i < storage.length; i++) {
      storage[i]()
    }
  }

  return fn => {
    storage.push(fn)

    if (!lock) {
      window.requestAnimationFrame(handler)
    }
  }
}