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

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

Closed
Zashy opened this issue Dec 29, 2016 · 25 comments
Closed

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

Zashy opened this issue Dec 29, 2016 · 25 comments

Comments

@Zashy
Copy link

Zashy commented Dec 29, 2016

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

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

Zashy commented Jan 3, 2017

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

Great! Think we should close out this issue then?

@mikestead
Copy link

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

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

mikestead commented Jan 14, 2017

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

ReDrUm commented Jan 23, 2017

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

landabaso commented Feb 17, 2017

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

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

landabaso commented Feb 17, 2017

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

developit commented Feb 17, 2017

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

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

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

@landabaso
Copy link

landabaso commented Feb 17, 2017

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 (#551, this one here, and another one I'll be investigating later).

Thanks for sharing this lib!

@developit
Copy link
Member

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

hanford commented Apr 22, 2017

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

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

hanford commented Apr 23, 2017

Great advice! Thanks again @developit

@Kagami
Copy link

Kagami commented Jul 5, 2017

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

@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!

louisremi added a commit to louisremi/react-toolbox that referenced this issue Aug 3, 2017
I managed to get react-toolbox to work with Preact (which was mostly a matter of customizing the Webpack config).
I've got one remaining problem that is due to some internal differences between React and Preact VDom libraries. From what I've gathered, components should avoid using node references is componentDidMount or componentDidUpdate without wrapping them in a `setImmediate` or `setTimeout(…, 0)` (see preactjs/preact#477 ).

With the current logic of componentDidUpdate in TimePickerDialog.js, the component throws a `TypeError: Cannot read property 'handleCalculateShape' of undefined` and interaction is impossible:


Fixing this in Preact seems like a difficult if not impossible task, however fixing it in the TimePicker component is only a matter of wrapping the call to `this.clockNode.handleCalculateShape`in an arrow function so that this.clockNode is not accessed immediately on component update.

Let me know if the change is acceptable :-)
louisremi added a commit to louisremi/react-toolbox that referenced this issue Aug 24, 2017
I managed to get react-toolbox to work with Preact (which was mostly a matter of customizing the Webpack config).
I've got one remaining problem that is due to some internal differences between React and Preact VDom libraries. From what I've gathered, components should avoid using node references is componentDidMount or componentDidUpdate without wrapping them in a `setImmediate` or `setTimeout(…, 0)` (see preactjs/preact#477 ).

With the current logic of componentDidUpdate in TimePickerDialog.js, the component throws a `TypeError: Cannot read property 'handleCalculateShape' of undefined` and interaction is impossible:


Fixing this in Preact seems like a difficult if not impossible task, however fixing it in the TimePicker component is only a matter of wrapping the call to `this.clockNode.handleCalculateShape`in an arrow function so that this.clockNode is not accessed immediately on component update.

Let me know if the change is acceptable :-)
@zaraza325632
Copy link

@marvinhagemeister
Copy link
Member

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

@Kamarudeen-k
Copy link

Kamarudeen-k commented Apr 17, 2020

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

developit commented Apr 17, 2020

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants