surevine / govuk-react-jsx

govuk-frontend compatible React components
https://govuk-react-jsx.netlify.app/
MIT License
3 stars 0 forks source link

Radios/Checkboxes: error in console due to null ref prior to render #132

Closed stachow closed 2 years ago

stachow commented 2 years ago

In all other similar instances in the codebase a check is made that the ref is not null before initialising the underlying govuk-frontend JS, e.g.

if (accordionRef.current) {
  new AccordionJS(accordionRef.current).init();
}

The null check is missing in the following two places, giving errors in the console when the code runs before first render:

https://github.com/surevine/govuk-react-jsx/blob/94d4ec6013188cee8e267c380060300deaf5eb26/src/utils/Boolean.js#L44

https://github.com/surevine/govuk-react-jsx/blob/94d4ec6013188cee8e267c380060300deaf5eb26/src/utils/Boolean.js#L56

andymantell commented 2 years ago

Thanks @stachow - I will take a look at this soon. Are you managing ok for a second if I don't get to this for a few days?

andymantell commented 2 years ago

Interesting, this doesn't seem to happen for me. Would you be able to put together a minimal reproduction if it's not too much trouble? No worries if not, but it might speed up a fix if you are able to.

andymantell commented 2 years ago

are you doing something like initialising this component without any items, but then loading in the items later e.g. as the result of an async call? So you get that first empty render happening without any checkboxes present?

stachow commented 2 years ago

Cheers for the response. This one is very edge-casey. Due to a bug in my code, the radios are put in to the DOM on page load, then instantly removed (thereby upsetting the internal JS leading to the console error). A repro is below. This is no longer an issue for me as the fix in my code prevents this initial "micro" rendering of the radios.

import { Radios } from "govuk-react-jsx";
import { useEffect, useState } from "react";

function App() {
  const [status, setStatus] = useState("idle");

  useEffect(() => {
    if (status === "idle") {
      // For error to appear in console, this delay must be tiny. If >=20ms on my machine, 
      //  then no error.
      setTimeout(() => setStatus("loading"), 5);
    }
    if (status === "loading") {
      setTimeout(() => setStatus("loaded"), 300);
    }
  }, [status]);

  return (
    <div className="App">
      {
        /* 
        Bug in my code on the following line: 
        - lifecycle of status is "idle" -> "loading" -> "loaded"
        - following check should be status === "loaded" to prevent radios being rendered 
           when status === "idle", then whipped out of DOM when "loading" (leading to console error)  
           then reinserted when "loaded"
        */
        status !== "loading" && (
          <Radios
            name="r"
            items={[{ reactListKey: "1", value: "1", children: "foo" }]}
            onChange={() => {}}
          ></Radios>
        )
      }
    </div>
  );
}

export default App;
stachow commented 2 years ago

BTW - as you may have guessed we are investigating using your library (nice work!), if you search for me on the gov digital slack you'll see which department. Cheers

andymantell commented 2 years ago

ah thanks for explaining the scenario @stachow. Glad that you've fixed around it for now, but I agree, it would be nice if these components were able to handle such a situation. Should be an easy enough fix - I'll try and get it into the next release when I do the govuk-frontend@4.x upgrade.

andymantell commented 2 years ago

Hi @stachow. This is now fixed in govuk-react-jsx@7.0.0