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

Add @parcel/css to benchmark #101

Merged
merged 3 commits into from
Jan 13, 2022
Merged

Add @parcel/css to benchmark #101

merged 3 commits into from
Jan 13, 2022

Conversation

onigoetz
Copy link
Contributor

@onigoetz onigoetz commented Nov 24, 2021

Hello,

I added @parcel/css as a benchmark.
It's an alpha but interestingly it's the fastest tool in every test.

Fixes #100

lib/minify.js Outdated
@@ -37,6 +38,9 @@ const minifiers = {
},
'esbuild': source => {
return esbuild.transformSync(source, { loader: 'css', minify: true }).code;
},
'@parcel/css': source => {
return parcelCss.transform({filename: "", code: Buffer.from(source), minify: true}).code;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to get even smaller output, you can specify browser targets. This allows @parcel/css to e.g. remove vendor prefixes that aren't needed. See here for an example. Maybe it could be a separate column for that similar to the "advanced" modes of the other tools if you want to keep it separate. However, unlike those advanced modes, removing prefixes is a safe transform.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I copied from the example benchmark you have in the repository. I wanted the default entry in the benchmark to be similar to other tools as they don't have any browser-specific optimisation and thus I didn't know which browsers would be "reasonabe" to set.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I definitely think it should be clearer from the benchmark results that many of the smallest sizes shown are using unsafe transforms though. Maybe something to work on separately.

@onigoetz
Copy link
Contributor Author

onigoetz commented Jan 8, 2022

Hi @XhmikosR, is there something I can help with to get this merged ?

package.json Outdated
@@ -31,6 +31,7 @@
"main": "lib/index.js",
"dependencies": {},
"devDependencies": {
"@parcel/css": "^1.0.0-alpha.4",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be updated to "@parcel/css": "^1.0.0", now that it has been released

@dominikg
Copy link
Contributor

note: with 1.0.0 i got errors on some input files

parcel err SyntaxError: Unexpected token Delim('*')
    at Object.@parcel/css (/home/dominikg/develop/css-minification-benchmark/lib/minify.js:45:21)
    at Object.measure (/home/dominikg/develop/css-minification-benchmark/lib/minify.js:97:48)
    at measure (/home/dominikg/develop/css-minification-benchmark/lib/index.js:23:30)
    at /home/dominikg/develop/css-minification-benchmark/lib/index.js:51:34
    at _fulfilled (/home/dominikg/develop/css-minification-benchmark/node_modules/q/q.js:854:54)
    at /home/dominikg/develop/css-minification-benchmark/node_modules/q/q.js:883:30
    at Promise.promise.promiseDispatch (/home/dominikg/develop/css-minification-benchmark/node_modules/q/q.js:816:13)
    at /home/dominikg/develop/css-minification-benchmark/node_modules/q/q.js:624:44
    at runSingle (/home/dominikg/develop/css-minification-benchmark/node_modules/q/q.js:137:13)
    at flush (/home/dominikg/develop/css-minification-benchmark/node_modules/q/q.js:125:13)

@XhmikosR
Copy link
Collaborator

Happy to include this assuming tests pass.

@dominikg
Copy link
Contributor

dominikg commented Jan 13, 2022

the errors are caused by outdated css invalid css syntax to please IE7 eg. *zoom: 1 in some of the data files.

Maybe you could update the pool to only include modern sets @XhmikosR
Or add error-handling in a way that it skips over and marks the error on the benchmark results (would skew total runtime results a bit tho).

edit: report on parcel-css repo parcel-bundler/lightningcss#39

@XhmikosR
Copy link
Collaborator

Not sure about it, it will need time which I don't have right now, plus the other minifiers work with it even if it's invalid per the specs.

@onigoetz
Copy link
Contributor Author

I updated to @parcel/css 1.0.0 Indeed two files are in error, I changed the code a bit to correctly display that those are in error (the isValid() check didn't return false for the checks in error)

@onigoetz
Copy link
Contributor Author

Fixed the warnings from the build as well

lib/minify.js Outdated
@@ -37,6 +38,9 @@ const minifiers = {
},
'esbuild': source => {
return esbuild.transformSync(source, { loader: 'css', minify: true }).code;
},
'@parcel/css': source => {
return parcelCss.transform({ filename: '', code: require('buffer').Buffer.from(source), minify: true }).code;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the require at the top

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, done

@XhmikosR
Copy link
Collaborator

I'm not sure the error changes are right. It seems it affects the total values too?

@onigoetz
Copy link
Contributor Author

It does but not too badly :

║ TOTAL - 3462799 bytes              │ 2752429 bytes - 1862.21 ms │ 2737446 bytes - 2287.01 ms  │ 2720884 bytes - 6105.36 ms │ 2509596 bytes - 5797.12 ms │ 2447799 bytes - 1514.23 ms │ 2755245 bytes - 625.9 ms       │ 2764893 bytes - 303.51 ms │ errorerror

It just concatenates the "error" labels, but still excludes the minifier that has at least one error

before it was printing something like NaNundefinederrorundefinederrorundefinedundefinedundefined

@XhmikosR
Copy link
Collaborator

We need this solution to be more robust, although I don't have a suggestion right now...

I'll just remove the offending CSS files.

@XhmikosR XhmikosR merged commit 52bd9dd into GoalSmashers:master Jan 13, 2022
@XhmikosR
Copy link
Collaborator

Thanks! If you have time to look at any other issues or improve the error situation, feel free to CC me (I don't watch repositories 'cause I'd go totally crazy 😛)

@XhmikosR
Copy link
Collaborator

And BTW, @parcel/css is super fast, almost unbelievable! I need to try it out.

@onigoetz
Copy link
Contributor Author

Yes I'll have a look into this, I'm working on a postcss plugin for @parcel/css right now and I'll come back to this after.

No worries, your mental health is more important than open source projects ;-)

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

Successfully merging this pull request may close these issues.

Add parcel-css
4 participants