Code cleanup, DRY

[sirian]

Look at https://github.com/jsdom/cssstyle/blob/master/lib/allExtraProperties.js https://github.com/jsdom/cssstyle/blob/master/lib/allProperties.js

var allExtraProperties = new Set();
module.exports = allExtraProperties;
allExtraProperties.add('background-position-x');
allExtraProperties.add('background-position-y');
allExtraProperties.add('background-repeat-x');
allExtraProperties.add('background-repeat-y');
allExtraProperties.add('color-interpolation');
...  // ~750 lines of same code in 2 files

Why not simply pass all of them in Set constructor? Or at least use loop

module.exports = new Set([
    'background-position-x',
    'background-position-y',
    'background-repeat-x',
    'background-repeat-y',
...
]);

UPD. or using arrays

var props = [
    'background-position-x',
    'background-position-y',
    'background-repeat-x',
    'background-repeat-y',
...
];

for (let i = 0; i < props.length; i++) {
    allExtraProperties.add(props[i])
}

These 2 files now takes 30kb of code. And 50% of this size is overhead of repeated 250 times allExtraProperties.add() and 500 times extraProperties.add()

[fatfisz]

As far as I know the package is used in setups which still support IE 11, where the new Set(iterable) form is not working. So instead of requiring polyfills all the way down the line, it's easier to just do away with calling .add for each thing.

There's no other reason for that.

[sirian]

Ok... But still code could be optimized using array

var props = [
    'background-position-x',
    'background-position-y',
    'background-repeat-x',
    'background-repeat-y',
...
];

for (let i = 0; i < props.length; i++) {
    allExtraProperties.add(props[i])
}

allExtraProperties.add(); takes 25 additional bytes per line and repeats ~240 times = 6KB. Whole file takes 11.5kb. So... 50% overhead

UPD. same problem with https://github.com/jsdom/cssstyle/blob/master/lib/allProperties.js where is also 500 lines of duplicated code

[sirian]

@jsakas take a look at PR #105

[jsakas]

@fatfisz hmm... is there a case for using this package in a browser? AFAIK this package is meant to be used in a nodejs environment only - as to replicate existing functionality you get in a browser. I don't know why we would try to support IE.

[fatfisz]

I honestly don't remember right now and for me it's not something super important, so I don't think we have to support IE. IMO we can just wait until someone complains and even then only add a note to README about the need to polyfill Set.

One thing though: with changes such as this that don't really add any new functionality and are only about a perceived optimization, I'd love to have some benchmarks or some clear goals for the project, e.g. if the project aims to be as small as possible or if it aims to be as fast as possible.

With all that said, my gut feeling tells me that the best optimization is to put everything into new Set([...]) and to drop the loops.

[sirian]

@fatfisz @jsakas BTW. found another huge overhead - coverage dir. It takes 1.5MB. Is it necessary to include this dir in package?

bash-3.2$ npm install jsdom
bash-3.2$ du -d 1 -k .
22388	./node_modules
22416	.
bash-3.2$ du -d 1 -k node_modules/cssstyle
108	node_modules/cssstyle/node_modules
20	node_modules/cssstyle/scripts
480	node_modules/cssstyle/lib
1468	node_modules/cssstyle/coverage
2108	node_modules/cssstyle

jsdom package downloads 22mb of code, and 1.5mb of this is cssstyle/coverage folder

[fatfisz]

@sirian Good find, could you make a separate PR for that? Adding "lib" to "files" in package.json should do the job.

[sirian]

@sirian Good find, could you make a separate PR for that? Adding "lib" to "files" in package.json should do the job.

https://github.com/jsdom/cssstyle/pull/106

[sirian]

I honestly don't remember right now and for me it's not something super important, so I don't think we have to support IE. IMO we can just wait until someone complains and even then only add a note to README about the need to polyfill Set.

One thing though: with changes such as this that don't really add any new functionality and are only about a perceived optimization, I'd love to have some benchmarks or some clear goals for the project, e.g. if the project aims to be as small as possible or if it aims to be as fast as possible.

With all that said, my gut feeling tells me that the best optimization is to put everything into new Set([...]) and to drop the loops.

Updated PR https://github.com/jsdom/cssstyle/pull/105 using Set constructor

[jsakas]

@sirian I have merged your changes into master to reduce the file size, but then realized that they are overwritten when running the download script npm run download. It pulls the properties from https://www.w3.org/Style/CSS/all-properties.en.json and generates the js file.

Oversight on my part (its late) so we need to fix this script to use new Set syntax I can publish a new version of the package.

This probably means that webkit and default properties will end up being the same file again.