Support for Maps in isEmpty function. (refactoring)

[webpapaya]

Line 93 should check if the styles are empty or not. Until now the isEmpty function only supports objects and no Maps. So this PR adds a Map support to the isEmpty function and replaces:

if (!isMap && isEmpty(styles)) continue;

with

if (isEmpty(styles)) continue;

[danieljuhl]

I'm not sure I get the reason for this. Could you please provide a sample of your use case?

[webpapaya]

It's about making the code simpler and more explicit. If you're reading

if (!isMap && isEmpty(styles)) continue;

As far as I understood the code it wants to check if something is empty or not. If it is empty it should just continue. So with this PR you can simply use:

if (isEmpty(styles)) continue;

In the end you don't care if it's a Map and Object or an Array of styles, you care about if it is empty or not.

[webpapaya]

also the isEmpty method was untested.

[danieljuhl]

@webpapaya Yes, I see that isEmpty was untested, and I fully get you code. What I would like to know, is why we need to support Maps? Can you provide a sample of code, that gives me an idea of why Stilr should support Maps.

[webpapaya]

"The entries() method returns a new Iterator object that contains the [key, value] pairs for each element in the Map object in insertion order." - from MDN

So for CSS where later declared styles might overwrite previously defined styles, it really makes sense to have things rendered in the order of insertion.

[danieljuhl]

@webpapaya You are still not answering my question :) I know what a Map is, I'm asking for a sample of a use case, where you would create a StyleSheet from a Map. I fully get what your changes does, but I don't see why you would want to change Stilr. Please provide a reason / sample code / anything, that explains to me, why this pull request is needed, and which problem it solves.

[webpapaya]

This PR is not a new feature. For me it just enhances the readability of the code. The internals of stilr are heavily using a map Line 9. Line 13 is explicitly checking if the stylesheet passed in to the create function is a map. So the stylesheet param in Line 83 is a map if it is a media query.

Also it safes an unnecessary recursion call to render function in Line 96 if the media query styles are empty. And for me calling isEmpty is more explicit/better to read than isMap && isEmpty(styles).

[danieljuhl]

Okay, but is there a real impact? Performance, stability, anything? Or is this simply another code style?

[webpapaya]

A slight performance improvement, which is negligible. It's more a side effect of the code style improvement, which should help people contributing to this stilr understand the code better.

[danieljuhl]

Where do you see the performance improvement? Do you have any benchmark to back this? From reading the code, I would guess the opposite as there is more function calls involved.

And to be honest, I prefer object instanceof Map over isIstanceOfMap(object). The amount of chars is almost the same, but by reading the code I don't know for sure what isIstanceOfMap does, as it is hidden a function - which also involves an extra function call.

To sum it up, I don't think there is any improvements, but if you have a benchmark that says otherwise, I'm willing to have a second look. I also don't agree that the code style is improved, I actual think it is harder to read, as thinks are hidden in functions as mentioned above.

[danieljuhl]

Adding tests for isEmpty() is fully acceptable.. and should be added, I agree.

[webpapaya]

We can probably argue about the name isInstanceOfMap, because it's telling technical detail and not what it actually does. Maybe isStylesheet might be better (good names are hard). But wrapping this functionality in a function makes it much easier to change/reuse and refactor using an IDE.

The amount of chars is almost the same, ...
It's not about chars and I agree that isInstanceOfMap isn't the best name, but even if a small functionality has a longer name than the actual implementation of this functionality it might be better to just wrap it up in a function with a solid name as a replacement for a comment/explanation for the functionality.

For me it's more confusing to have an isEmpty util function, which doesn't support Maps as they are used as the stylesheet must be a map. If it would be called isEmpty would be called isObjectEmpty (as it's only supporting js objects right now) then it wouldn't be confusing for somebody contributing to stilr.

For me this line is really confusing:

if (!isMap && isEmpty(styles)) continue;

As it mixes up technical detail 'isMap' with busines logic isEmpty(). If you're just looking at this line without knowing anything about the internals of stilr. Do you know what isMap is?

So we can also argue about the name of isMap right? So in the above given context we can probably rename it to isMediaQuery. What do you think?