fre: Missing clean-up of refs

It looks like neither callback refs nor object refs are getting cleaned up.

In the case of callback refs, this means you can’t use a callback to clean up any side-effects - here’s an example:

const DatePicker = () => {
  const init = el => {
    if (el) {
      // initialize date-picker using some third-party plain JS lib...
    } else {
      // clean up any date-picker pop-ups that may still be open...
    }
  }
  return <span ref={init}/>
}

The clean-up call never happens, so we currently can’t really rely on refs for this sort of thing.

Note that this may not be a very useful feature in a codebase that exclusively uses hooks, but if we want to support class-based components at some point down the line, many class-based libraries rely on refs for side-effects and clean-up.

I noticed earlier that object refs also do not get cleaned up - so if you have something like this:

const Component = () => {
  const item = useRef()
  const [showItem, setShowItem] = useState(true)

  // ...

  return (
    <div>
      {showItem && <div ref={item}/>}
    </div>
  )
}

If showItem gets updated to false, item.current will still point to the DOM element, which gets removed from the DOM, but now it can’t be garbage-collected.

As always, I also compared the behavior with preact, and it also deviates from react on this point, so I have opened an issue here as well.

As explained in that issue, I am not actually sure if what I reported there is a bug in preact or react, since preact appears to behave more accurately as it’s described in the React docs.

Until we know if the behavior regarding null updates is correct in react or preact, I would actually advise you wait to address this issue - there is definitely an issue in fre either way, since it doesn’t remove references at all, but at the moment I couldn’t tell you precisely if react or preact dispatches the null updates correctly, so lets wait and see.

For comparison, here’s the output from all three:

Note that, besides the missing null updates, it also looks like callback refs are also currently being dispatched twice during every update in fre.

It could be you actually meant for one of those calls to be the null update after clearing out the internal reference? That would seem to bring the behavior very close to that of react as it behaves currently.

You might to address that detail right away, as well as the null updates missing after removal.

Or just wait and see who’s right between react and preact first. 😏

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 28 (12 by maintainers)

Commits related to this issue

add test for callback refs: this test currently fails and contains a TODO; pending further discussion, see #97 — committed to mindplay-dk/fre by mindplay-dk 5 years ago

Most upvoted comments

I don’t understand that example. What element would the ref function be receiving here? <App> is not an element, it’s a functional component, so in React, you would receive the component class instance - but since this library is hooks-only, there are no component classes.

Refs exist primarily to obtain references to DOM elements - with functional components and hooks, this is primarily useful in conjunction with effects, and effects often depend on refs.

Does its sub elements need to perform cleanup?

Yes, definitely. Refs themselves can have direct effects on the DOM (or other global state) and these types of effects aren’t complete without a clean-up cycle - just as useEffect would not be very useful if it didn’t support clean-up.

I look forward to discussing completely different results from react and modifying the test

The point of having tests is to define expectations - they’re an executable specification of what you intend to build. You can change the tests, if the specification changes, but you need to know why you’re changing them; you can’t just change the tests to match what the library does, not without knowing precisely what you want the library to do.

I can go into this at length, but it's off-topic to this discussion, so I'll leave the rest of my comments as a collapsible - click here. 🙂

It's never a good idea to commit changes that break the tests, because you lose track of anything else you might break before you get the tests working again.

It’s a much better idea to change the tests first - this documents the change you’d like to make to the specification, and at that point, you know why the tests are failing and can work towards fixing the code to match the spec.

It’s also not a good idea to commit breaking changes directly to master - if you’re going to change the tests, you should always do so on a separate branch, so the changes to the spec and library can be reviewed and tested together.

I’m not opposed to deviations from React, but you shouldn’t be changing (or breaking) the tests unless the requirements have changed. Tests are not an afterthought to show how something works - you should think of them as a plan to show how something should work.

In other words, “the test is always right” - if the test fails, it should be because you changed the test, never because you changed the library. (unless you discover that the test itself contains a bug, which does of course also happen.)

This is not to say I think my tests are always right! 😉

But since you didn’t have any tests, we had to start somewhere, and I’m basing these tests on what I’ve learned from React, Preact, and a handful of other libraries of this type. If I had merely written tests that matched what your library already did, we wouldn’t have found or corrected many of the bugs, so you can probably already see how this works. Again, my tests may not align with your expectations, but then you need to describe your expectations with tests before changing the library, otherwise the tests have no real value.

I understand you’re new to testing, just trying to help 🙂

This article summarizes the principles of test-driven development:

The principles of TDD are as follows:

Tests are always written before the code that will make them pass. The test anticipates the correct behavior of the code.

Development proceeds one test at a time. Once a test goes from failing to passing, the next test is written and development can continue.

Again, tests should be as simple as possible, only long enough to break the application in its current state. After the test is written, all development must focus on making the software pass the test.

TDD proceeds in cycles which are usually called “red, green, refactor.” This cycle is usually done once for each unit test that you write. The cycle consists of three stages:

Red: Write a unit test that fails due to missing functionality in the software (since the color red usually denotes failure).

Green: Write code that fixes the issue by making the software pass the test (since the color green usually denotes success).

Refactor: Clean up the code base to account for the presence of this new code.

As with anything, there are no “universal truths” about software development, but these principles are mostly unchallenged - I promise, you will be happier, more successful and more productive, if you learn to follow these principles. 😊

mindplay-dk on Nov 12, 2019

Yes, definitely. Refs themselves can have direct effects on the DOM (or other global state) and these types of effects aren’t complete without a clean-up cycle - just as useEffect would not be very useful if it didn’t support clean-up.

If so, I need to refactor it again.

And about tests, I would like to comply with the tes, after we discuss the results, I will try to update the tests and make a report.

yisar on Nov 12, 2019

I’ve confirmed the fix, looks right. 👍

Let’s keep this issue open until we have a test to prove it. (Blocked by #91)

mindplay-dk on Nov 20, 2019

Thank you very much. I think this problem and #87 should exist at the same time. I will fix them as soon as possible 💯 .

yisar on Oct 29, 2019