New property optimizer (3rd attempt)

[Venemo]

(Third attempt.)

This pull request contains a new property optimizer which can perform advanced optimalizations on a CSS selector body. It fixes the following issues: #173, #134, #168, #164, #184, #190, #210

Changes since the previous pull request:

• Dozens of new unit tests added
• Fixed batch tests
• Improved performance
• Added a good fix for Property merging works incorrectly if the properties are not declared consecutively #210 and similar issues

Remaining work:

• Enhance the new property optimizer to completely replace the old one. At this moment it just runs an additional step.
• Support for font, animation, transition and border to Shorthand properties #134

[tomByrer]

Wow, you did alot of work, looks like you created a v3 rewrite! Well commented also, though you may be flogged publicly for using w3schools & not MDN or CSS-Tricks ;) Might be better to remove those schooling references; makes too much visual noise in the code, bandwidth, & I would hope devs know how to use a search engine.

I'll have to test later.
BTW, I noticed you removed/changed a background:.... repeat test, is there one or two in there? Seemed most were no-repeat.

[GoalSmashers]

+1 to getting rid of w3schools references

[Venemo]

@tomByrer, @GoalSmashers Yeah, I know... http://www.w3fools.com/ I forgot to remove the w3schools links, and looking at the code again, seems I also forgot to remove some TODO comments. Unfortunately, most of the people don't know how to use a search engine, so I'd prefer to have some links anyway, maybe to a more appropriate place. Is MDN okay to link to?

@tomByrer I have added tests for background with both repeat and no-repeat, but I can add more if you have something specific in mind.

Btw, this is not really a rewrite. It's still the same concept, only a bit more complicated because now it doesn't forget about what was a shorthand in the original input and what wasn't. So you can think of it as several iterations over the previous code, but all squashed into a single commit.
The previous iteration made the (wrong) assumption that a shorthand is always equivalent to its granular components, but apparently this is not the case when there are parts that the browser doesn't understand.

[Venemo]

One more thing: the code could be organized better. Do you think it's a good idea to put some of the functionality into separate files?

[GoalSmashers]

1200 lines is indeed a bit too much. Feel free to iterate over it if you have spare time and it's doable.

On 28 Feb 2014, at 08:12, Timur Kristóf notifications@github.com wrote:

One more thing: the code could be organized better. Do you think it's a good idea to put some of the functionality into separate files?

—
Reply to this email directly or view it on GitHub.

[tomByrer]

MDN okay to link to?

Perhaps in the WiKi. Links in code are usually to odd bug reports or unusual syntax (like rare color codes or a programming trick). Code usually is not a place to teach beginner 'programmers' who don't know what a search engine is. Either way, even though w3schools is now thought to be "more friendly to beginners than existing documentation sites", MDN, the actual w3c specs, css-tricks, & sometimes StackOverflow are considered more credible.

You could make a blog post explaining what you did, & teach/link CSS & JS you used. Could get you upvoted on Reddit & HN.

1200 lines is indeed a bit too much

Yea, aside from I suspect Lo-Dash, most code is split up if it goes over a few pages long. Perhaps split out the longer functions if they make sense alone? Just please don't split into 30 files with 6 lines each; I hate reading those repos.

You're doing great stuff @Venemo !

[Venemo]

Thanks for the compliments!

I'll split some of the stuff into multiple modules, the code will be a lot more readable / understandable. About links: you've convinced me, I'll just remove them.

[tomByrer]

Welcome :)
Links like that I usually put in the push remark; to prove why the change is needed, & that I'm doing it the most correct way.

BTW, I think your code is understandable now. Smaller chunks are usually easier to manage & mentally process.

[Venemo]

Yes, that's what I meant. Splitting into multiple modules usually also helps with maintainability for the same reasons it helps to more easily grasp the code mentally.

[Venemo]

Okay, I think I'm done :)

[GoalSmashers]

Please put all requires on top.

[GoalSmashers]

Most of the comments are regarding:

• Code formatting (the way we use braces project-wide)
• Caching .length calls
• Guard pattern
• Having require calls always on top

But in general awesome work! 💯

[tomByrer]

@GoalSmashers While some of the things you do may be standard for a majority of JS devs, some are not. Perhaps it is best to educate via a style guide, since it seems jsHint won't catch everything? Plus, there are many like me who are willing to learn new best practices, like your checking values via an array & indexOf.

[Venemo]

@GoalSmashers @tomByrer Thanks for the thorough review! I can implement the suggestions later today.

[GoalSmashers]

@Venemo - great, looking forward to it!

@tomByrer - that's a good idea - I'll make sure that all JSHint-enforced and other rules are there

[Venemo]

Some personal notes:

• Guard pattern - I agree with it, makes the code more readable
• require on top - same here
• Caching .length - I did it because you asked, but it's not always doable. (I have loops when the length changes during each iteration). Generally, I think this is a micro-optimalization and should be done by the JIT compiler, not me.
• } else { - I did this too because the project uses this convention but I hate these TIE fighter elses because I think they make the code harder to read.

Anyway, all the asked changes are implemented.

[Venemo]

@GoalSmashers Is there anything left before you can merge the PR?

[tomByrer]

Thanks for the fancy formatting @GoalSmashers

Caching .length - is a micro-optimalization

Actually does not matter anymore except in IE, so I guess it is already JIT optimized, at least in tight, repeated loops?

} else { ...these TIE fighter elses because I think they make the code harder to read.

"TIE fighter" is funny. I thought that was the standard, so I tweaked my Notepad++ editor to make those loops easier to find:

[GoalSmashers]

@Venemo - it should be good enough. I'll go through it for the last time and merge it unless I find any issues.

@tomByrer @Venemo - both are project's conventions and making a 'TIE fighter' (funny indeed) binds instructions together imo.

[Venemo]

@GoalSmashers Okay! Btw, no disrespect meant for the project conventions! :)

[GoalSmashers]

👍 no worries!

[tomByrer]

congrats @Venemo

[Venemo]

Thanks @tomByrer it wasn't easy :) If you have any feedback about the stuff, I'm always interested to hear about you! :)