preactjs / preact

⚛️ Fast 3kB React alternative with the same modern API. Components & Virtual DOM.
https://preactjs.com
MIT License
36.62k stars 1.95k forks source link

in ref callback, ref is someitimes not part of the DOM yet #477

Closed Zashy closed 5 years ago

Zashy commented 7 years ago

Not sure if this is supposed to work this way or not, but it seemed odd to have the ref only work sometimes.

I've got a div with a ref callback, and using that ref I wanted to get it to initialize swipejs which requires a width from the ref's children. However, I found that the ref is sometimes not yet part of the DOM. So when it tries to access the width on the page within the ref callback sometimes the width is zero, and sometimes it is my expected width of ~1000px. I dug in, and saw the width being 0, document.querySelectorAll('.swipe') being [], and ref.parentNode being null during the times it wasn't working. Other times those all returned expected results of the width, the ref, and the parent.

Didn't have time to dig in too deep, but I did find a workaround by just grabbing the ref in the callback, and then using that ref in the componentDidMount lifecycle hook.

Here's my code with the working workaround

import { h, Component } from 'preact'
import SwipeJS from 'swipejs'

class Example extends Component {
  constructor (props) {
    super(props)
    this.state = {
      activeIndex: 0
    }
  }

  componentDidMount() {
    const ref = this.swipeRef
    if (ref) {
      this.swipe = this.swipe || new SwipeJS(ref)
    }
  }

  componentWillUnmount () {
    if (this.swipe) {
      this.swipe.kill()
      this.swipe = false
    }
  }

  initializeSwipe (ref) {
    if (!ref) {
      // ref is called on removal with null element
      return
    }
    this.swipeRef = ref
  }

  render (props, state) {
    return (
        <div className='swipe' ref={ref => this.initializeSwipe(ref)}>
          <div className='swipe-wrap'>
            <div></div>
            <div></div>
          </div>
        </div>
    )
  }
}

export default Example
developit commented 7 years ago

Hi there - preact doesn't guarantee refs are called after parent elements are mounted. Instead (and in general in React codebase), I recommend instead using the ref callback to store a reference to the created element, then using the componentDidMount() lifecycle event to initialize your swipe system.

Zashy commented 7 years ago

Hi thanks, yeah that is what I ended up doing. I couldn't find the documentation in react on how exactly that was supposed to work. Glad to know that's expected behavior.

developit commented 7 years ago

Great! Think we should close out this issue then?

mikestead commented 7 years ago

@developit Just running into an issue which is related to ref callback I think when trying out blueprintjs EditableText component with Preact. I need to investigate some more, but can you clarify whether Preact follows the same callback pattern as React here i.e.

The ref attribute takes a callback function, and the callback will be executed immediately after the component is mounted or unmounted https://facebook.github.io/react/docs/refs-and-the-dom.html

Asking here as may be related to this issue. Sorry if you answered above, I wasn't totally clear on the answer.

developit commented 7 years ago

Currently Preact does not defer ref callback invocation like it does componentDidMount(). If that breaks enough things then maybe we'll need to look into doing it. I've never run into anything until this issue, but maybe it was a result of people not realizing there was a difference there.

mikestead commented 7 years ago

Great thanks for clarifying! I've not come across any issues around this before either, but for reference here's one example of where it seems breaking. https://github.com/palantir/blueprint/blob/master/packages/core/src/components/editable-text/editableText.tsx#L123

With Preact focus is not given to the input, I assume because it's not attached.

Also with Preact, that ref callback seems to be getting called every time I type a char in the input, I've not had time to debug that one yet.

ReDrUm commented 7 years ago

This stumped me for a while too. Below is an example of some code where Preact requires I refactor it to ensure the event listeners are correctly added.

onReference = (iframe) => {
    if (this.viewport) {
        this.viewport.removeEventListener('hashchange', this.onHashChange);
    }
    this.viewport = iframe ? iframe.contentWindow : undefined;
    if (this.viewport) {
        this.viewport.addEventListener('hashchange', this.onHashChange, false);
    }
};

render() {
    return <iframe ref={this.onReference} />;
}
landabaso commented 7 years ago

I'm having the same issue. I was assuming the ref callback to be triggered after mount/unmount.

I cannot fix my problem easily using componentDidMount. See one possible case:

  render () {
    const clonedChildren = React.cloneElement(this.props.children, {ref: this.onChildrenIsMounted});
    return <SomeHoC>
      <SomeHoC2>
        {clonedChildren}
      </SomeHoC2>
    </SomeHoc>;

That is, clonedChildren may not be mounted at the same time the parent component is mounted depending on SomeHoC... And I don't have a handler for the clonedChildren componentDidMount.

developit commented 7 years ago

Hmm - @landabaso that doesn't seem any different from React's behavior. ref is invoked when the element is added to the DOM, not the virtual DOM. If a hoc defers rendering of the DOM element, the ref is deferred as well.

landabaso commented 7 years ago

Yes, the ref is deferred. That works Ok. But when the callback is triggered, clonedChildren has not been attached to the DOM yet.

I need to access the .parentNode of the clonedChildren.

I can access .parentNode of the element in this.onChildrenIsMounted when using React. It's not the case with Preact. It looks like it has not been attached to the DOM. Is it possible?

developit commented 7 years ago

It's something I'm open to, just haven't had time to explore. Technically, refs should not be used to pierce component boundaries - that is what's causing the issue here - so the number of times this has come up is only twice. Happy to explore options though, just it's not something I've ever run into myself. I rarely use refs.

landabaso commented 7 years ago

I admit it's a strange situation. I'd usually pass a callback to the HoC. In this case I cannot pass a callback to the HoC that renders the clonedChildren because the HoC is a 3rd party lib :-/

developit commented 7 years ago

ahh yeah. the other case I saw for this was in a third party lib as well.

landabaso commented 7 years ago

I guess this will happen more often as more users try to use preact.

There's a workaround. I can defer the call to onChildrenIsMounted using setTimeout(..., 0). It's a hack, but not super-terrible I guess...

So far I only had 3 issues porting my App to preact (https://github.com/developit/preact/issues/551, this one here, and another one I'll be investigating later).

Thanks for sharing this lib!

developit commented 7 years ago

Nice! I was going to suggest the setTimeout hack, but I didn't know if it would fix your usecase. Glad there's at least a workaround for now. We'll get #551 solved, that was just an oversight on my part.

hanford commented 7 years ago

@developit

I think this is one of the big reasons I'm unable to switch a big application over to preact is the fact that refs aren't a 1:1 with react.

I've been building a little react component over here: https://github.com/hanford/react-drag-drawer and lines like this break. In react, this.drawer (a ref) will always be defined, but they're always not defined in preact.

Any workarounds or ideas?

developit commented 7 years ago

I would definitely avoid invoking DOM side effects from within render like this - instead, use componentDidUpdate() / componentDidMount():

componentDidMount() {
  if (this.state.open) {
   this.attachListeners();
  }
}

componentDidUpdate(prevProps, prevState) {
  if (this.state.open && !prevState.open) {
    this.attachListeners();
  }
}

Ideally, you want render() to be a pure VDOM pipeline, it should never trigger any external effects.

hanford commented 7 years ago

Great advice! Thanks again @developit

Kagami commented 7 years ago

Hi, I think I have similar issue but I didn't fully understand the conversation above.

I have this test code:

class Test extends Component {
  setRef = (el) => {
    console.log("Setting ref to " + el);
    //if (!el) return;
    this.el = el;
  }
  showA = () => {
    this.setState({show: true});
  }
  closeA = () => {
    this.setState({show: false});
  }
  showEl = () => {
    console.log(this.el);
  }
  updateEl = () => {
    this.el.textContent = new Date().getTime().toString();
  }
  renderA() {
    const { show } = this.state;
    if (!show) return null;
    return (
      <div>
        A <button onClick={this.closeA}>Close A</button>
      </div>
    );
  }
  render() {
    return (
      <div>
        {this.renderA()}
        <div ref={this.setRef} />
        <button onClick={this.showA}>Show A</button>
        <button onClick={this.showEl}>Show el</button>
        <button onClick={this.updateEl}>Update el</button>
      </div>
    );
  }
}

Reproducing steps: click Show A, click Close A, click Update el. Console output:

Setting ref to [object HTMLDivElement]
Setting ref to null
Setting ref to [object HTMLDivElement]
Setting ref to [object HTMLDivElement]
Setting ref to null

So after closing A element my ref callback is being called twice and second time with null, so I can't use DOM element anymore. If I uncomment the if line then everything seems to be working though.

Could you please explain this behavior? What is best way to store correct reference to DOM element in Preact?

developit commented 7 years ago

@Kagami you're missing a key prop on the conditionally rendered <div> in renderA(). Without a key or a component reference, that <div> and the <div> with the ref are being swapped. In VDOM, the last element of a given type is always removed, unless keys are used to indicate which needs to be removed.

// render after clicking "Show A":
<div>
  <div>A <button onClick={this.closeA}>Close A</button></div>  <!-- from renderA -->
  <div ref={this.setRef} />
  <button onClick={this.showA}>Show A</button>
  <button onClick={this.showEl}>Show el</button>
  <button onClick={this.updateEl}>Update el</button>
</div>

// render after clicking "Close A":
<div>
  <div ref={this.setRef} />
  <button onClick={this.showA}>Show A</button>
  <button onClick={this.showEl}>Show el</button>
  <button onClick={this.updateEl}>Update el</button>
</div>

Since there's nothing to indicate which div is which, it's removing the second one instead of the first. Making <RenderA> a component or assigning <div key="a"> to the div returned from renderA() will fix the issue.

Cheers!

zaraza325632 commented 6 years ago

marvinhagemeister commented 5 years ago

We completely rewrote the ref handling in Preact X and fixed this :tada: An alpha release is already available on npm :100:

Kamarudeen-k commented 4 years ago
import React from 'react';
import Menu from '../icons/Menu.png';

export default class IconButton extends React.Component{
    constructor(){
        super();

        this.icon = null;
        this.login = null;

        this.handleMouseOver = this.handleMouseOver.bind(this);
    }

    handleMouseOver(toggle){
        console.log("handleMouseOver Called");

        let background = "#034d79";
        let foreground = "white";
        if(toggle === true){
            background = "white";
            foreground = "#034d79";           
        }
        console.log("icon: "+this.icon);
        if(this.icon != null){    
            this.icon.setAttribute("background-color", background);
            this.icon.setAttribute("color", foreground);
        }
        if(this.login != null){
            this.login.setAttribute("background-color", background);
            this.login.setAttribute("color", foreground);
        }

    }

    render(){
        return (
            <div align="right" className="div-inline">
                <img src={Menu} alt="Icon" name="icon" align="center" ref={node => {this.icon = node;}} onMouseEnter={this.handleMouseOver(true)} onMouseLeave={this.handleMouseOver(false)} className="login-icon"></img>
                <button name="login" align="right" ref={node => {this.login = node;}} onMouseEnter={this.handleMouseOver(true)} onMouseLeave={this.handleMouseOver(false)} className="login-button">Login</button>
            </div>             
        );
    }

}

The ref holder variable shows null! ? Could someone help me understand why this null error is?

Can I set the attributes like this?

developit commented 4 years ago

@Kamarudeen-k you are calling your mouse handler functions in render() when you should only be passing references to them. This has nothing to do with refs, it's a mistake in your JavaScript code.

- <foo onClick={this.foo(true)}>
+ <foo onClick={this.foo.bind(this,true)}>

Here is the corrected code:

        ​return​ (
            ​<​div align​=​"​right​"​ className​=​"​div-inline​"​>​
                ​<​img src​=​{Menu} alt​=​"​Icon​"​ name​=​"​icon​"​ align​=​"​center​"​ ref​=​{​node​ ​=>​ {​this​.​icon​ ​=​ node;}} onMouseEnter​=​{​this​.​handleMouseOver​(​true​)} onMouseLeave​=​{​this​.​handleMouseOver​(​false​)} className​=​"​login-icon​"​><​/​img​>​
                ​<​button name​=​"​login​"​ align​=​"​right​"​ ref​=​{​node​ ​=>​ {​this​.​login​ ​=​ node;}} onMouseEnter​=​{​this.handleMouseOver.bind(​this,true)} onMouseLeave​=​{​this.handleMouseOver.bind(this,false)} className​=​"​login-button​"​>​Login​<​/​button​>​
            ​<​/​div​>​             
        );
Kamarudeen-k commented 4 years ago

Thank you for your quick response. yes, It works with its reference