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

Support for React-like refs? #184

Open
mcjazzyfunky opened this issue Mar 28, 2021 · 23 comments · May be fixed by #185
Open

Support for React-like refs? #184

mcjazzyfunky opened this issue Mar 28, 2021 · 23 comments · May be fixed by #185
Labels
enhancement New feature or request

Comments

@mcjazzyfunky
Copy link

mcjazzyfunky commented Mar 28, 2021

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
Copy link
Owner

What's the fully requested functionality?

@jorgebucaran jorgebucaran added the enhancement New feature or request label Mar 29, 2021
@mcjazzyfunky
Copy link
Author

mcjazzyfunky commented Mar 29, 2021

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

What's the fully requested functionality?

I basically meant that:

  • In analogy of React, both ref objects { current: null } and ref callbacks (elem) => {...} should be supported.
  • Also the corresponding ref object should be cleared (=> elemRef.current = null) or the ref callback should be called with null (=> elemCallback(null)) when the ref property has changed or the corresponding element has been disconnected.

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
Copy link
Author

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

@jorgebucaran
Copy link
Owner

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
Copy link
Author

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):
js-works/js-element@a776770

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
Copy link
Owner

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
Copy link
Author

@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):
js-works/js-element@480db0c

@jorgebucaran
Copy link
Owner

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.

mcjazzyfunky added a commit to mcjazzyfunky/superfine that referenced this issue Mar 30, 2021
mcjazzyfunky added a commit to mcjazzyfunky/superfine that referenced this issue Mar 30, 2021
mcjazzyfunky added a commit to mcjazzyfunky/superfine that referenced this issue Mar 30, 2021
@jorgebucaran jorgebucaran linked a pull request Mar 30, 2021 that will close this issue
@jorgebucaran
Copy link
Owner

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
Copy link
Author

mcjazzyfunky commented Mar 30, 2021

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
Copy link
Owner

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

@whaaaley
Copy link
Contributor

whaaaley commented Apr 19, 2021

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
Copy link
Author

mcjazzyfunky commented Apr 19, 2021

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
Copy link
Owner

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

@whaaaley
Copy link
Contributor

whaaaley commented Apr 19, 2021

@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
Copy link
Author

@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 😉
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
Copy link
Contributor

whaaaley commented Apr 19, 2021

@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
Copy link
Owner

@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
Copy link
Contributor

whaaaley commented Apr 23, 2021

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
Copy link
Author

mcjazzyfunky commented Apr 25, 2021

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
Copy link
Owner

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

@mcjazzyfunky
Copy link
Author

mcjazzyfunky commented Apr 27, 2021

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
Copy link
Contributor

whaaaley commented Jun 26, 2021

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)
    }
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants