peterbe / minimalcss

Extract the minimal CSS used in a set of URLs with puppeteer
https://minimalcss.app/
MIT License
353 stars 35 forks source link

ignoreJSErrors true, yet puppeteer throws in some cases. #400

Open ggiak opened 4 years ago

ggiak commented 4 years ago

Expected Behavior

When ignoreJSErrors is true to get a minimal css result.

Current Behavior

minimalcss throws an exception. image

Steps/Code to Reproduce

const minimalcss = require('minimalcss')
let options = {
    urls: ["https://hostchefs.eu"],
    enableServiceWorkers: false,
    ignoreCSSErrors: true,
    ignoreJSErrors: true,
    ignoreRequestErrors: true,
    puppeteerArgs: ['--no-sandbox'],    
};
minimalcss.minimize(options)
  .then(result => {
      console.log( result.finalCss ) 
  })
  .catch(error => {
    console.log(`Oups: ${error}`)
    process.exit(1)
  })
peterbe commented 4 years ago

I'm confused. Those are two separate errors (one ReferenceError (which I can easily see in the web console on https://hostchefs.eu/) and TypeError). And why does it not say Oups: in your output?

peterbe commented 4 years ago

So, the first error, ReferenceError is actually just the ignored JS error being logged to stdout. The other, TypeError, is actually an error happening deep inside csso. You can see the whole stacktrace if you use console.error like this:

let options = {
  urls: ['https://hostchefs.eu'],
  enableServiceWorkers: false,
  ignoreCSSErrors: true,
  ignoreJSErrors: true,
  ignoreRequestErrors: true,
  puppeteerArgs: ['--no-sandbox'],
};
minimalcss
  .minimize(options)
  .then((result) => {
    console.log(result.finalCss);
  })
  .catch((error) => {
    console.log('Something wrong wrong');
    console.error(error);
    process.exit(1);
  });

The full error is:

[Error: ReferenceError: FWDRLS3DUtils is not defined
    at https://hostchefs.eu/:909:4]
Something wrong wrong
TypeError: Cannot read property 'some' of undefined
    at TRBL.getValueSequence (/Users/peterbe/dev/JAVASCRIPT/minimalcss/node_modules/csso/lib/restructure/4-restructShorthand.js:81:51)
    at TRBL.attemptToAdd (/Users/peterbe/dev/JAVASCRIPT/minimalcss/node_modules/csso/lib/restructure/4-restructShorthand.js:170:31)
    at TRBL.add (/Users/peterbe/dev/JAVASCRIPT/minimalcss/node_modules/csso/lib/restructure/4-restructShorthand.js:232:23)
    at List.<anonymous> (/Users/peterbe/dev/JAVASCRIPT/minimalcss/node_modules/csso/lib/restructure/4-restructShorthand.js:353:38)
    at List.eachRight (/Users/peterbe/dev/JAVASCRIPT/minimalcss/node_modules/css-tree/lib/common/List.js:181:12)
    at Object.processRule (/Users/peterbe/dev/JAVASCRIPT/minimalcss/node_modules/csso/lib/restructure/4-restructShorthand.js:335:25)
    at Object.enter (/Users/peterbe/dev/JAVASCRIPT/minimalcss/node_modules/csso/lib/restructure/4-restructShorthand.js:423:53)
    at Object.<anonymous> (/Users/peterbe/dev/JAVASCRIPT/minimalcss/node_modules/css-tree/lib/walker/create.js:11:16)
    at List.walkNode (/Users/peterbe/dev/JAVASCRIPT/minimalcss/node_modules/css-tree/lib/walker/create.js:166:19)
    at List.eachRight (/Users/peterbe/dev/JAVASCRIPT/minimalcss/node_modules/css-tree/lib/common/List.js:181:12)

For the record, csso is used inside minimalcss thru the use of css-tree which is a CSS parser that parses the found external CSS files. Lastly, I think after the AST tree has been manipulated by minimalcss, it gets serialized with csso.

What's curious is A) why isn't minimalcss mentioned in the stacktrace?! B) What is the bug in csso??

ggiak commented 4 years ago

Hey @peterbe, I share the same concerns.

I also tried this against puppeteer 5.3 so I can use chromium 8x instead of 7x but the same error applies.

Thought I should open an issue in case you have seen something similar before.

I will also ask for a css validation on the following: https://jigsaw.w3.org/css-validator/validator?uri=https%3A%2F%2Fhostchefs.eu%2F&profile=css3svg&usermedium=all&warning=1&vextwarning=&lang=en

so that we have additional information what creates the issue.

peterbe commented 4 years ago

For the record, I dug into it a bit and concluded that:

ggiak commented 4 years ago

For the record, I dug into it a bit and concluded that:

  • Upgrading to csso@4.0.3 and/or css-tree@1.0.0-alpha.39 does not solve the problem.

Forgot to mention I have already tried this as well.

peterbe commented 4 years ago

I dumped all the CSS it downloads to a temp directory:

▶ ls -l *.css
-rw-r--r--  1 peterbe  wheel  209164 Sep 24 09:42 all.min.css
-rw-r--r--  1 peterbe  wheel    3938 Sep 24 09:42 cookieconsent.min.css
-rw-r--r--  1 peterbe  wheel   52479 Sep 24 09:42 custom.css
-rw-r--r--  1 peterbe  wheel    4272 Sep 24 09:42 default_theme.css
-rw-r--r--  1 peterbe  wheel  170570 Sep 24 09:42 fontawesome-all.min.css
-rw-r--r--  1 peterbe  wheel   16848 Sep 24 09:42 hc.css
-rw-r--r--  1 peterbe  wheel    3634 Sep 24 09:42 owl.carousel.css
-rw-r--r--  1 peterbe  wheel    1152 Sep 24 09:42 owl.theme.css
-rw-r--r--  1 peterbe  wheel    6626 Sep 24 09:42 responsive.css
-rw-r--r--  1 peterbe  wheel     914 Sep 24 09:42 skin_classic_white.css

In response.css you have this:

.products-res{float:left;margin-right:55px width:50%}

And I put some console.log in node_modules/csso/lib/restructure/4-restructShorthand.js:81 and print the declaration when declaration.value.children === undefined and it prints:

DECLARATION {
  type: 'Declaration',
  loc: null,
  important: false,
  property: 'margin-right',
  value: { type: 'Raw', loc: null, value: '55px width:50%' },
  id: 514,
  length: 27,
  fingerprint: null
}

So when theres a mention of 55px width:50% it croaks. And that's mentioned in your CSS validator too.

Now, I wish we can turn this into a bug on csso and css-tree because it shouldn't crash if you have invalid CSS. At least not this late.

peterbe commented 4 years ago

By the way, there's something really wrong about that response.css by the way. I get an error trying to run prettier --write response.css which means it can't parse it either.

This happens. We (CSS files) all make mistakes. I just wish the error wasn't so cryptic. I'd love it if minimalcss could throw a helpful error like:

throw new Error(`Unable to continue because ${url} could not correctly be parsed to an AST`);

Do you think you can take a look at that? I suspect what happens is that minimalcss correctly manages to parse it but, when you try to use csso.syntax.compress(ast) it fails.

In fact, this diff kinda solves your problem:

diff --git a/src/run.js b/src/run.js
index 69ae7b0..1637585 100644
--- a/src/run.js
+++ b/src/run.js
@@ -115,6 +115,13 @@ const processStylesheet = ({
   stylesheetContents,
 }) => {
   const ast = csstree.parse(text);
+  try {
+    csso.syntax.compress(ast);
+    console.log(`${responseUrl} WORKED!\n`);
+  } catch (ex) {
+    console.log(`${responseUrl} FAILED HORRIBLY!\n`, ex);
+  }
+
   csstree.walk(ast, (node) => {
     if (node.type !== 'Url') return;
     const value = node.value;

Obivously, that's not good enough for a fix. But might be a start. At least, equipped with something like this you'd get an insight where to put your attention (responsive.css in your case).

Also, remember that you can potentially move this code down into the bottom of the file where you have an Object (stylesheetAsts) that maps each URL to an AST.

Perhaps something like --debug-css-urls could kick in some extra code that scrutinizes each AST like this. Do you think you can take a look at that? If you get stuck, I can help write a unit test.