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

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

Open
ggiak opened this issue Sep 24, 2020 · 7 comments
Open

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

ggiak opened this issue Sep 24, 2020 · 7 comments

Comments

@ggiak
Copy link

ggiak commented Sep 24, 2020

##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
Copy link
Owner

peterbe commented Sep 24, 2020

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
Copy link
Owner

peterbe commented Sep 24, 2020

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
Copy link
Author

ggiak commented Sep 24, 2020

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
Copy link
Owner

peterbe commented Sep 24, 2020

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

  • Upgrading to [email protected] and/or [email protected] does not solve the problem.
  • Error, from minimalcss's perspective happens in the line csso.syntax.compress(allCombinedAst, cssoOptions);

@ggiak
Copy link
Author

ggiak commented Sep 24, 2020

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

Forgot to mention I have already tried this as well.

@peterbe
Copy link
Owner

peterbe commented Sep 24, 2020

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
Copy link
Owner

peterbe commented Sep 24, 2020

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.

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

No branches or pull requests

2 participants