Error solution: Uncaught TypeError

Question

Uncaught TypeError: document.getElementById(...) is null

I have a single JavaScript file that is connected to multiple pages. The below snippet is a function that is used for a single page. The above error appears when the user navigates to one of the pages that does not use the below function, and it references the document.getElementById('button01').addEventListener('click', newThing); line at the bottom. I've gathered that the error comes up due to the fact that button01 does not exist on these pages.

function newThing() {
const output = document.getElementsByTagName('output')[0];
if (!(document.forms.thingSelection2.type.value in options)) {
    return false;
}
const list = options[document.forms.thingSelection2.type.value];
const method = document.forms.thingSelection1.mode.value + 'Item';
const item = list[method]();
output.innerHTML = item;

}
document.getElementById('button01').addEventListener('click', newThing);

Solution

My solution for this is simple; place the line in an if statement like so:

if(document.getElementById('button01')) {
    document.getElementById('button01').addEventListener('click', newThing);
}

This removes the error from the pages that don't use it.

Questions

Does this open up the possibility for any buggy behavior that I don't know about? Is there a better way to achieve the same results?

HTML

I'm thankful for the feedback this post has received thus far. Since some of the answers contain recommendations based on assumptions of my HTML, I've decided to add the respective HTML in the below snippet. One thing to note is that I use two forms. This is required for the functionality of the code. I'm unaware of any unintended bugs this may cause. If it does, please let me know below.

<div><output></output></div>
<div><button id="button01">New Thing</button></div>
<div>
   <form name="thingSelection1">
      <input type="radio" name="mode" value="random" id="mode1">&nbsp;Random
      <br/><input type="radio" name="mode" value="forward" id="mode2">&nbsp;Old&nbsp;-&nbsp;New
      <br/><input type="radio" name="mode" value="reverse" id="mode3">&nbsp;New&nbsp;-&nbsp;Old
   </form>
</div>
<div>
   <form name="thingSelection2">
       Doodle&nbsp;<input type="radio" name="type" value="doodle" id="doodleCheck"><br/>
       Video&nbsp;<input type="radio" name="type" value="video" id="videoCheck"><br/>
       Audio&nbsp;<input type="radio" name="type" value="audio" id="audioCheck"><br/>
       Photo&nbsp;<input type="radio" name="type" value="photo" id="photoCheck"><br/>
       Text&nbsp;<input type="radio" name="type" value="text" id="textCheck">
   </form>
 </div>

Furthermore, I'd like to point out that the original snippet is only part of my full JavaScript. Based on the information provided by this post, I've concluded that adding the remaining JavaScript, while possibly beneficial and relevant to some of the answers below, could possibly be out of the scope of the original topic of this post. Please note your thoughts on this in the comments below.

Answer 1

Does this open up the possibility for any buggy behavior that I don't know about?

It's not likely to. The if statement is fine, though it can be made cleaner:

• Rather than selecting the element twice (once to check to see if it exists, another time to call addEventListener on it), save it in a variable:

  const button = document.getElementById('button01');
  if (button) {
    button.addEventListener('click', newThing);
  }
  

• Or, if you're writing JS which gets transpiled for production (which, in a professional or larger project, you really should be), use optional chaining:

  document.getElementById('button01')?.addEventListener('click', newThing);
  

But the fundamental issue remains - the HTML layout is completely disconnected from the JavaScript. Having to check to see if a button exists in the first place is something that ideally shouldn't be an issue to worry about, at least in a larger or more professional project. What if you had not 1, but 3 or 5 or 10 elements with handlers on different pages, all of which may or may not exist? The codebase would be more difficult to maintain than it should be.

There are a few solutions for this:

• One option is to have a separate <script> file for pages with the form, eg:

<form id="thingSelection2">
...
</form>
<script src="./thingSelection.js"></script>

where thingSelection.js adds the event listener. But that'll require a separate request to the server, which may be a problem on a large page on HTTP 1.1 - if you have lots of different scripts like this, the sheer number of parallel requests could slow things down, especially for users with high latency. (The HTTP/2 protocol doesn't have issues with additional connections to the same server IIRC)

(You could also inline the script, eg </form><script>// etc</script>but I prefer putting scripts in separate files to permit caching)

• But if it were me, I'd strongly prefer to fully integrate the creation of the HTML with the event listeners for that HTML, to make it completely inconceivable of one existing without the other, using a framework. For example, with React, you could do something along the lines of:

const Things = () => {
    const [item, setItem] = useState('');
    const [selectedOption, setSelectedOption] = useState('foo');
    const clickHandler = (e) => {
        const fn = options[selectedOption];
        if (fn) {
          setItem(fn());
        }
    };
    return (
        <div>
            <select value={selectedOption} onChange={e => setSelectedOption(e.currentTarget.value)}>
                <option value="foo">foo</option>
                <option value="bar">bar</option>
            </select>
            <button onClick={clickHandler}>click</button>
            <output>{item}</output>
        </div>
    );
};

const options = {
  foo: () => 'foo',
  bar: () => 'bar',
};

const Things = () => {
    const [item, setItem] = React.useState('');
    const [selectedOption, setSelectedOption] = React.useState('foo');
    const clickHandler = (e) => {
        const fn = options[selectedOption];
        if (fn) {
          setItem(fn());
        }
    };
    return (
        <div>
            <select value={selectedOption} onChange={e => setSelectedOption(e.currentTarget.value)}>
                <option value="foo">foo</option>
                <option value="bar">bar</option>
            </select>
            <button onClick={clickHandler}>click</button>
            <output>{item}</output>
        </div>
    );
};
ReactDOM.render(<Things />, document.querySelector('.react'));

<script crossorigin src="https://unpkg.com/react@16/umd/react.development.js"></script>
<script crossorigin src="https://unpkg.com/react-dom@16/umd/react-dom.development.js"></script>
<div class="react"></div>

Then when you're on a page where the Things are needed, you just render <Things />. With this approach, the HTML its associated JS handlers are all completely self-contained in the Things. There's no need to use selectors that may have unintentional collisions. (For example, going with your original code of document.getElementById('button01'), what if some other completely separate section of the HTML on one of the pages had an element which accidentally used the same ID? Then you'll have problems.)

Using a framework like this is admittedly a lot to learn, but it makes codebases significantly more maintainable. It's well worth it for medium-size or larger projects, IMO.

---

On a different note, your current code could also be improved a bit:

Prefer selectors Selector strings are usually more easily understood and more flexible than other methods of selecting elements (like getElementsByTagName and document.forms.someFormName.someFormElement). The selector string that matches an element will line up with the CSS selector that styles the element. For example, I'd replace:

document.forms.thingSelection2.type.value

with

document.querySelector('#thingSelection2 [name=type]').value

and

const output = document.getElementsByTagName('output')[0];

with

const output = document.querySelector('output');

(no need to select a collection when you only need the single matching element)

Save the value Instead of selecting and extracting the value twice, write DRY code; put it into a variable:

const { value } = document.querySelector('#thingSelection2 [name=type]');
if (value in options) { // Or, use `!options[value]
  return false;
}

Answer 2

It looks like you incorporated some of the advice from my answer to one of your other posts - that is nice.

The answer by CertainPerformance has great advice. Going along with the essence of it, there are other parts of the code that "could" be problematic:

const output = document.getElementsByTagName('output')[0];

While there should be an <output> element if the HTML code has one, if the JavaScript executes before the DOM is ready then this could lead to an error just as the code to get an element by id attribute would:

const output = document.getElementsByTagName('output')[0];
console.log('output tagName:', output.innerHTML)

It would be best to ensure that document.getElementsByTagName('output') has a non-zero length before accessing the first element.

---

Another suggestion is to return as early as possible - e.g. the line to assign output has no affect on the conditional:

const output = document.getElementsByTagName('output')[0];
if (!(document.forms.thingSelection2.type.value in options)) {
    return false;
}

While it likely makes no difference for this code whether the output element is fetched from the DOM, reducing computations is a good habit to develop. In other situations it could save valuable time for users - e.g. take the example of a server-side request to fetch data, which might take a few seconds.

Answer 3

I assume your HTML looks a bit like this:

<output></output>
<output></output>
<form>
  <input name="mode" value="..." />
  <input name="type" value="..." />
  <button>Click!</button>
</form>

My core recommendation would not be to rely on IDs as they are unique per page. Instead, consider relying on data attributes, and never assuming that there are a particular number of them. Your javascript should be as disconnected from your HTML layout as possible.

Let's modify what you have a bit. Let's start with adding a data attribute that indicates to JavaScript that you have a special button:

<button data-my-widget>Click!</button>

This would enable you to assign an event listener to all buttons with the given attribute and would enable your buttons to remain progressively enhanced:

const myThing = event => {
  ...
}

for (const button of document.querySelectorAll('[data-my-widget]')) {
  button.addEventListener('click', newThing);
}

You are also hard-coding which form each button applies to - and the respective controls - within the event handler. You can make this a bit better by using the form attribute on a button, and by providing the target element ids as data attributes as well.:

<form>
  <input name="mode" value="...." />
  <input name="type" value="...." />
  <button data-my-widget data-mode="mode" data-value="type">Click!</button>
</form>

Then you could access this form within the handler:

const myThing = event => {
  const outputs = document.getElementsByTagName('output');
  if (outputs.length === 0) {
    return;
  }

  const output = outputs[0];
  const target = event.target;
  const { form } = target;
  if (form === undefined) {
    return;
  }

  const { mode: modeId, value: valueId } = target.dataset;
  const mode = form.elements[modeId]?.value;
  const value = form.elements[valueId]?.value;
  if (mode === undefined || value === undefined || value in options === false) {
    return;
  }

  const list = options[value];
  const f = list[`${mode}Item`];
  if (f === undefined) {
    return;
  }
  output.innerHTML = f();
}

This keeps your javascript as unaware of your HTML structure as possible, although I'd still recommend replacing the outputs section as well, potentially by using data attributes, rather than relying on the index of the element:

<output id="output-1"></output> 
<button ... data-output="output-1">Click!</button>

const myThing = event => {
  ...
  const { mode: modeId, value: valueId, output: outputId } = target.dataset;
  ...
  const output = document.getElementById(outputId);
  output.innerHTML = ...;
}

Putting this all together:

<output id="output-1"></output>
...
<output id="output-n"></output>
<form>
  <input name="mode" value="...." />
  <input name="type" value="...." />
  <button data-my-widget data-mode="mode" data-value="type" data-output="output-1">Click!</button>
</form>

const handleClick = event => {
  const { form } = event.target;
  if (form === undefined) {
    return;
  }

  const { mode: modeId, value: valueId, output: outputId } = target.dataset;
  const mode = form.elements[modeId]?.value;
  const value = form.elements[valueId]?.value;
  const output = document.getElementById(outputId);
  if (mode === undefined || value === undefined || output === undefined) {
    return;
  }

  const list = options[value];
  const f = list[`${mode}Item`];
  if (f === undefined) {
    return;
  }

  output.innerHTML = f();
}

for (const button of document.querySelectorAll('[data-my-widget]')) {
  button.addEventListener('click', handleClick);
}