30-seconds-of-code: arrayToHtmlList update

[skatcat31]

Description

So our function arrayToHtmlList is in a pretty bad space and could use an update. We already had some discussion over at #675 but I wanted to move the update discussion over to an issue for more visibility.

Current problem:

It uses an awkward query selector instead of the document.prototype.getElementByID() function, and it uses an IIFE to declare an intermediate variable closure when we could do with just API chaining the return values.

Another issue is we are just forcing it inside a list item. We could easily allow a formatter to be passed as the last argument with the map footprint and a default that won't change functionality, however the change in footprint because a bit... awkward as it becomes data, binding, callback, but it was an old monster to begin with.

With those two issues in mind we should look at either updating or cleaning up the snippet so it is more maintainable than it's current form or decide to leave as it.

[siarhei-zharnasek]

const arrayToHtmlList = (arr, selector, elemTag) =>
  (
    container => container.innerHTML += arr.reduce((acc, el) => `${acc}<${elemTag}>${el}</${elemTag}>`, '')
  )(document.querySelector(selector));

@skatcat31 what do you think about implementing this way?

[skatcat31]

@Siarhei-Zharnasek I feel like if we're going to do that we should do a formatter function so they can choose their own entire string. You also don't need an inner closure, you can just chain the selection. We did however choose by ID so we need to keep that footprint unless we want a breaking release

[Chalarangelo]

• +1 on document.selectElementById. Either that or we should pass a query selector and use document.querySelector instead. Either way, it's awkward right now.
• IIFE has to go, let's do something better instead.
• I like the formatter idea. From a usability perspective, it's a lot better. From a learning standpoint, this might land the snippet in advanced (depending on verbosity), but it's a very good teaching opportunity.

TL;DR: Let's fix that, it's an awkward snippet and all the proposed suggestions are pretty good.

[Ankitb09]

const arrayToHtmlList = (arr, id, elemTag) => { let parentElem = document.getElementById(id); parentElem.innerHTML += arr.map((item) => { return<${elemTag}>${item} }).join('') }

@skatcat31 What do you think about this?

[skatcat31]

@Ankitb09 again what advantage does that bring over adding a default formatter so they can handle the full string if wanted?

[konstantin-hatvan]

Hello,

this is my first time contributing to open source. This issue seemed like a good one.

What do you think about this?

const arrayToHtmlList = (arr, listID, formatterString = '<li>|</li>', marker = '|') => {
  document.querySelector(`#${listID}`).innerHTML += arr.map(item => formatterString.replace(marker, item)).join('');
};

Example Usage:

arrayToHtmlList(['item 1', 'item 2'], 'listId'); // use default template string
arrayToHtmlList(['item 1', 'item 2'], 'listId', '<li class="test">This is the added item: |</li>'); // only use default replace marker but overwrite template string
arrayToHtmlList(['item 1', 'item 2'], 'listId', '<li class="test">This is the added item: ###</li>', '###'); // overwrite template string and replace marker --> full flexibility

Because default parameters are used we don't have to use the extra flexibility available but we can choose to do so (e.g. in case we have to use the default marker somewhere in the string as output and therefore have to overwrite it with a custom marker).

Another posiibility would be to use a DOM element instead of a string (e.g. by creating it with document.createElement('LI')) ...

Option 2 with formatter function:

const arrayToHtmlList = (arr, listID, formatterFunction) => {
  document.querySelector(`#${listID}`).innerHTML += arr.map(item => formatterFunction(item)).join('');
};

Example Usage:

arrayToHtmlList(['Item 1', 'Item 2'], 'listId', item => `<li class="myClass">${item}</li>`);

I prefer this option because we have full power over how the output is generated. The only downside to this is that we do not have a default parameter for the formatter function. Sure, we could define it inside the arrayToHtmlList function and then check if a function was provided.

[skatcat31]

@konstantin-hatvan your example 2 can be cleaned up a little bit:

const arrayToHtmlList = (arr, listID, formatterFunction = (item) => `<li>${item}</li>`) => {
  document.getElementById(listID).innerHTML = arr.map(formatterFunction).join('');
};

This is very close to what I suggested earlier, and since most people seem to be settling on this format, and since I have time I'll be opening a PR with this version since it does not break functionality, and adds an extra optional for formatting using recent language features.

However one last thing I keep noticing:

Do we overwrite the innerHTML or do we concatenate to it? I would think we'd overwrite since then it gives a clean representation, and each call is a known 'formats the way we want', but before I open the PR I would like to settle so we don't have any confusion

[konstantin-hatvan]

@skatcat31 I tried your example in the browser console and the default parameter for formatterFunction throws an error. I am not sure if this syntax is correct.

Also, I was wondering if it would make sense to use reduce() instead of map() since the array should return as a single value?

[skatcat31]

You're right I was missing a closing arguments parens. I have updated the previous suggestion to reflect the need.

as for your suggestion, Array#reduce() might be an option

const arrayToHtmlList = (arr, listID, formatterFunction = (acc, item) => ((acc +=`<li>${item}</li>`), acc)) => {
  document.getElementById(listID).innerHTML = arr.reduce(formatterFunction, '');
};

However we run into a lot of really nasty grouping in the footprint and having to keep around an empty string in body, which is something we need to do regardless(the Array#join()). It does however reduce the time complexity by a significant margin(O(n) vs O(2n))

This however means that any formatter passed will be a Array#reduce() function and thus must remember the accumulator, or in other words more developer debt versus operation debt.

So then the question becomes do we allow more time complexity or more developer complexity?

We could avoid both problems by using a custom for loop accumulator so that a map compatible function gets passed, manually applied, and then added to the base string, solving both problems.

const arrayToHtmlList = (arr, listID, formatterFunction = (item) => `<li>${item}</li>`) => {
  let list = '';
  for (let i = 0, l = arr.length; i < l; i++){
    list += formatterFunction(arr[i], i, arr);
  }
  document.getElementById(listID).innerHTML = list;
};

It is still fairly short and easy to remember, so that may be a plus. Sadly this is one of those instances where a for loop might actually be desirable.

[konstantin-hatvan]

@skatcat31 Although this is not the most aesthetic implementation, it's the best solution in my eyes.

I think it is preferable to overwrite the innerHTML rather than concatenating because of the cleaner representation, as you have mentioned before.

[Chalarangelo]

@skatcat31 Wanna rewrite this one as discussed? Or I could give it a shot in a few days.

[skatcat31]

@Chalarangelo I'm currenlty filling out a bunch of paper work for now. I would be able to rewrite it around the 19 or so

[nickforddesign]

I’d recommend making this a pure function that just returns html, leave the DOM out of it.

[Chalarangelo]

@nickforddesign I think that might be a good idea.

[skatcat31]

That's probably for the best. It means we'd be releasing a breaking change, but to be fair this thing is in a bad spot. If we REALLY wanted to we could actually returns a full nodelist so that other modifications could be done, but I think at that point a stand alone script library would be better

[skatcat31]

so yeah I'd be okay with it just returning an HTML string instead

[Chalarangelo]

If we return a HTML string, this will then be tagged something other than browser as the primary tag, probably array. Not an issue with me, I'm just adding this here as a reminder.

[freality]

Hey all!

Just found (and really like) your project! Great work.

I'd like to through an implementation into the ring. Essentially a modification of a few previous suggestions.

const arrayToHtmlList = (arr, listID, formatterFn = (item) => `<li>${item}</li>`) => {
    document.getElementById(listID).innerHTML += arr.reduce(
        (acc, curr, idx) => acc += formatterFn(curr, idx)
        , ''
    );
};

Also, it strikes me that by adding the default formatterFunction, this function effectively becomes appendArrayOfItemsToElement.

Example:

arrayToHtmlList(
    ['foo.js', 'bar.js', 'baz.js'], 
    'body-id', 
    (src) => `<script type="text/javascript" src="${src}"></script>`
);

[skatcat31]

Man this shows how busy a full time high cognitive load job gets. We should make a final decision on this soon and release the change

[samuelcolburn]

2 more suggestions:

1. The same as @skatcat31 's most recent Array.prototype.reduce() method but using Element.insertAdjacentHTML() instead of Element.innerHTML:

const arrayToHtmlList = (arr, listID, formatterFunction = (acc, item) => ((acc +=`<li>${item}</li>`), acc)) =>
  document
    .getElementById(listID)
    .insertAdjacentHTML('beforeend', arr.reduce(formatterFunction, ''));

Reasoning:

The insertAdjacentHTML() method of the Element interface parses the specified text as HTML or XML and inserts the resulting nodes into the DOM tree at a specified position. It does not reparse the element it is being used on, and thus it does not corrupt the existing elements inside that element. This avoids the extra step of serialization, making it much faster than direct innerHTML manipulation.

Source: MDN Element.insertAdjacentHTML

1. Similar to @nickforddesign 's suggestion, creates and returns new element instead of manipulating an existing element:

const arrayToHtmlList = (
  arr,
  tagName = 'ul',
  formatterFunction = (acc, item) => ((acc +=`<li>${item}</li>`), acc)
) => {
  const list = document.createElement(tagName);
  list.insertAdjacentHTML('beforeend', arr.reduce(formatterFunction, ''));
  return list;
};

[jasonralston100]

const arrayToHtmlList = (arr, listID, formatterFunction = (item) => <li>${item}</li>) => {
document.getElementById(listID).innerHTML = arr.map(formatterFunction).join('');
};

This is the easiest and cleanest way to take a constant array and convert it to an array list with it's inner.HTML. If it were PowerShell I would just use a HTML fragment for the formatting. Not sure what your gaining here. It should output just fine or you could convert to straight xml? this is just output <li>${item}</li>

[Rkv88]

Hi I would like to add something :
. using a classic for loop will be more efficient than a reduce function especially when converting big array .
by the way this is my first contribution on GitHub👶
also i'm writing a new snippet right now any suggestion other than what's mentioned in the contribution Guideline

[meysam81]

After reading through all the comments and the previous PR, I would also suggest returning just the HTML and letting the developer decide what to do with it. As the name also suggests "array to html list". It does not imply anything about changing the DOM.

peace :v:

[Chalarangelo]

@meysam81 I think we will probably stick with that and maybe add a blog for clarification/exploration on the subject.

[Chalarangelo]

I'm closing this and will probably add a blog soon to clarify and explore ideas from this thread.