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

Different formats of tag lists #5

Open
brokentone opened this issue Sep 14, 2016 · 3 comments
Open

Different formats of tag lists #5

brokentone opened this issue Sep 14, 2016 · 3 comments

Comments

@brokentone
Copy link

The reactHTMLTags and voidElementTags lists are very different in terms of structure, requiring different lookup methods in each list. I get one of them was lifted from external source, but it would substantially reduce confusion if each were either an array or an object with true values.

https://github.com/CondeNast/jsonmltoreact/blob/master/src/utils.js#L7-L165

@dcolucci
Copy link

That is a good point. We could switch them both to arrays, or we could switch them both to objects and then tweak the logic whereby we get the "converter" for each element type which starts here: https://github.com/CondeNast/jsonmltoreact/blob/master/src/utils.js#L171.

Probably switching to arrays is cleaner. Admittedly I was just copying React and thought there might be a good reason why they chose object, but probably that reason is something isolated to their coding aesthetic because there's no way the performance varies tangibly between object and array for something this small.

@pgoldrbx
Copy link

to the degree that we are able, I would suggest we conform to React where relevant. However, if it's impractical, consistency is still best.

@diffcunha
Copy link

If you look carefully, reactHTMLTags is only used to produce the reactConverters object:

export const reactConverters = reactHTMLTags.reduce((acc, type) => {
  acc[type] = type;
  return acc;
}, {});

Changing reactHTMLTags to an object would imply the usage of something as _.reduce in order to produce the same reactConverters. With an array, the native reduce is enough therefore less external dependencies and simpler code.

In regards to voidElementTags I believe it was added @dcolucci, IMO keeping it as an object seems fine because it simplifies the lookup, representing it as an array would imply a _.includes or some other helper function that would never run in constant time like the object solution (though is pretty much negligible in this case)

My suggestion is that we move voidElementTags to an array and apply reduce on it to produce an object:

const voidElementTagsList = [
  'area',
  'base',
  ...
];

export const voidElementTags = voidElementTagsList.reduce((acc, type) => {
  acc[type] = true;
  return acc;
}, {});

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

No branches or pull requests

4 participants