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 ignoreCssErrors option #249

Merged
merged 1 commit into from Aug 8, 2018

Conversation

Projects
None yet
3 participants
@stereobooster
Collaborator

stereobooster commented Aug 8, 2018

Not sure about name of the option

@stereobooster stereobooster changed the title from [WIP] Add ignoreCssError option to Add ignoreCssError option Aug 8, 2018

@stereobooster

This comment has been minimized.

Collaborator

stereobooster commented Aug 8, 2018

cc @jc275

@peterbe

Great work but I really think we should add that plural s on the name of the option.

const { finalCss } = await runMinimalcss('invalid-css', {
ignoreCssError: true
});
expect(finalCss).toEqual('');

This comment has been minimized.

@peterbe

peterbe Aug 8, 2018

Owner

Awesome!

README.md Outdated
@@ -176,6 +176,7 @@ key is `urls`. Other optional options are:
of strings for headless Chrome](https://peter.sh/experiments/chromium-command-line-switches/).
* `cssoOptions` - CSSO compress function [options](https://github.com/css/csso#compressast-options)
* `timeout` - Maximum navigation time in milliseconds, defaults to 30 seconds, pass 0 to disable timeout.
* `ignoreCssError` - ignore CSS errors. By default minimalcss reports all CSS errors, but if you want to ignore those errors, if you do not have access to those css files, you can set this option to true.

This comment has been minimized.

@peterbe

peterbe Aug 8, 2018

Owner

Sorry for being difficult but that should really be called ignoreCssErrors (with a s suffix) because the functionality is that it will ignore all CSS errors for the whole duration of the minimalcss run.

This comment has been minimized.

@peterbe

peterbe Aug 8, 2018

Owner

Also, can you uppercase the second instance of the word css to CSS. For consistency.

This comment has been minimized.

@stereobooster

stereobooster Aug 8, 2018

Collaborator

Yes sure. I was exactly expecting help on naming

README.md Outdated
@@ -176,6 +176,7 @@ key is `urls`. Other optional options are:
of strings for headless Chrome](https://peter.sh/experiments/chromium-command-line-switches/).
* `cssoOptions` - CSSO compress function [options](https://github.com/css/csso#compressast-options)
* `timeout` - Maximum navigation time in milliseconds, defaults to 30 seconds, pass 0 to disable timeout.
* `ignoreCssError` - ignore CSS errors. By default minimalcss reports all CSS errors, but if you want to ignore those errors, if you do not have access to those css files, you can set this option to true.

This comment has been minimized.

@peterbe

peterbe Aug 8, 2018

Owner

The - ignore CSS errors. part isn't super useful. The functionality is pretty clear from the name of the option (ignoreCssErrors).

I would change it to something like:

By default, any CSS parsing error throws an error in minimalcss. If you know it's safe to ignore (for example, third-party CSS resources), set this to true.
@stereobooster

This comment has been minimized.

Collaborator

stereobooster commented Aug 8, 2018

fixed

README.md Outdated
@@ -176,6 +176,7 @@ key is `urls`. Other optional options are:
of strings for headless Chrome](https://peter.sh/experiments/chromium-command-line-switches/).
* `cssoOptions` - CSSO compress function [options](https://github.com/css/csso#compressast-options)
* `timeout` - Maximum navigation time in milliseconds, defaults to 30 seconds, pass 0 to disable timeout.
* `ignoreCSSErrore` - By default, any CSS parsing error throws an error in minimalcss. If you know it's safe to ignore (for example, third-party CSS resources), set this to true.

This comment has been minimized.

@peterbe

peterbe Aug 8, 2018

Owner

typo :) "ignoreCSSErrore"

@peterbe

peterbe approved these changes Aug 8, 2018

I approve if you make that last typo fix :)
Feel free to land.

@stereobooster stereobooster merged commit c712e96 into peterbe:master Aug 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stereobooster stereobooster deleted the stereobooster:ignoreCssError branch Aug 8, 2018

@stereobooster stereobooster changed the title from Add ignoreCssError option to Add ignoreCssErrors option Aug 8, 2018

@peterbe

This comment has been minimized.

Owner

peterbe commented Aug 8, 2018

I guess I'll make a new release since this is a new feature.

@peterbe

This comment has been minimized.

Owner

peterbe commented Aug 8, 2018

0.7.8 released.

@jc275

This comment has been minimized.

Contributor

jc275 commented Aug 8, 2018

I spotted this far too late, but... what about ignoring JS errors too?

page.on('pageerror', error => {

@peterbe

This comment has been minimized.

Owner

peterbe commented Aug 9, 2018

@jc275 Do you have a solution in mind?

@jc275

This comment has been minimized.

Contributor

jc275 commented Aug 9, 2018

JS errors could be either logged or thrown, in exactly the same way that CSS errors are.

CSS-related errors might occur in unexpected places, but at least the on-page JS errors should all be found in one place, as 'pageerror' events.

I don't know whether there's any advantage to a separate JS error option, so maybe ignoreCssErrors could become ignoreErrors or allowErrors. (After all, if you're trying to extract CSS data from a third-party site, it doesn't really matter what caused the error, since you have no way to avoid it.)

@peterbe

This comment has been minimized.

Owner

peterbe commented Aug 10, 2018

It would be cool to have an ignoreJSErrors option for sure. If there's a clear and easy way to implement it so that it doesn't collide with our "own" error handling.

@jc275 jc275 referenced this pull request Aug 10, 2018

Merged

ignoreJSErrors #253

peterbe added a commit that referenced this pull request Aug 16, 2018

ignoreJSErrors (#253)
Aside from tests and documentation, this should be the only change needed.

Camel-cased `ignoreJsErrors` (and `ignoreCssErrors`) might be more in keeping with the [style guide](https://github.com/peterbe/minimalcss/blob/master/CONTRIBUTING.md#style-guide).

#249

jc275 added a commit to jc275/minimalcss that referenced this pull request Aug 17, 2018

ignoreJSErrors (peterbe#253)
Aside from tests and documentation, this should be the only change needed.

Camel-cased `ignoreJsErrors` (and `ignoreCssErrors`) might be more in keeping with the [style guide](https://github.com/peterbe/minimalcss/blob/master/CONTRIBUTING.md#style-guide).

peterbe#249
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment