Handle multiple semicolons to prevent csso crash, fixes #243 #244

jc275 · 2018-08-01T12:11:30Z

Fixes #243

There may be a safer way to handle this (e.g. via css-tree) rather than naively editing the CSS text... but this works for me :)

peterbe · 2018-08-01T12:28:12Z

src/run.js

@@ -218,6 +218,10 @@ const processPage = ({
          redirectResponses[responseUrl] = redirectsTo;
        } else if (resourceType === 'stylesheet') {
          response.text().then(text => {
+            // Double semicolons can crash csso.


Add a comment with a link to css/csso#378

peterbe

This needs a unit test. The unit test needs to also explain and mention css/csso#378

peterbe · 2018-08-01T15:44:46Z

tests/examples/extra-semicolons.css

@@ -0,0 +1,11 @@
+a {
+  color: blue;;;;background-color: red;;


Is it really necessary to test all of these different occurrences of double semi-colons? Isn't the error just going to happen on 1/the first of them?

peterbe · 2018-08-01T15:45:13Z

I'm guessing you're working on a unit test too.

jc275 · 2018-08-01T20:51:44Z

Whoops, guess I should probably have used some fancypants git commands to reduce the commit-spam :)

test will fail

stereobooster · 2018-08-08T17:27:00Z

src/run.js

+            // Semicolon sequences can crash CSSO, so we remove them.
+            // https://github.com/peterbe/minimalcss/issues/243
+            // https://github.com/css/csso/issues/378
+            while (/;\s*;/.test(text)) {


we have file src/utils.js you can place this function there with explanations in jsdoc

stereobooster · 2018-08-08T17:29:07Z

tests/main.test.js

+  // Extra semicolons can cause csso.minify() to throw:
+  // [TypeError: Cannot read property '0' of undefined]
+  // https://github.com/peterbe/minimalcss/issues/243
+  // https://github.com/css/csso/issues/378


I guess

const { finalCss } = await runMinimalcss('extra-semicolons'); expect(finalCss).toMatch('a{color:red}');

would be enough for test, no need check if exception undefined if we check return value

stereobooster · 2018-08-08T17:32:01Z

Can you please squash commits and change message to:

Handle multiple semicolons to prevent csso crash, fixes #243

unit tests added along with explanation about csso

jc275 · 2018-08-08T21:19:16Z

Thanks, @stereobooster. I've made the changes you suggested.

I'm not sure how to squash commits, though. Is that an option you can select when merging?

stereobooster · 2018-08-09T06:11:44Z

https://stackoverflow.com/a/5189600 this is what I meant by squash commits. And yes it is also possible to squash at merge via button in the interface

jc275 · 2018-08-09T19:44:21Z

I've been editing this directly on github (which no longer seems like a brilliant idea) so I don't have a local branch to rebase/squash. Can I leave the squashing to you?

peterbe · 2018-08-16T20:17:25Z

src/run.js

@@ -218,6 +218,7 @@ const processPage = ({
          redirectResponses[responseUrl] = redirectsTo;
        } else if (resourceType === 'stylesheet') {
          response.text().then(text => {
+            text = utils.removeSequentialSemis(text);


The wonderful comments you put above the utility function removeSequentialSemis belongs here instead. That utility function doesn't need a hack-explanation since it's just a function. A great function for that matter!

Also, when you move that comment can you make the URLs complete. I.e. add the https:// so that they become clickable URLs in editors and github.

peterbe · 2018-08-16T20:19:33Z

src/utils.js

+/**
+ * Semicolon sequences can crash CSSO, so we remove them from CSS text.
+ * github.com/peterbe/minimalcss/issues/243
+ * github.com/css/csso/issues/378


As mentioned above, these comments belong to where this function is used. But please put something else in here. The code could do with a really simple function like Replaces all occurances of a ';'<0 or more whitespace>';' to just a single ';'.

peterbe · 2018-08-16T21:00:34Z

src/utils.js

+    css = css.replace(/;\s*;/g, ';');
+  }
+  return css;
+};


Now, if you feel like you want to perfect this PR, since that string function got uplifted into its own function, can you add a simple test for it.

At the moment the utils.test.js doesn't exist but it soon will hopefully.

I do feel like perfecting this PR, but I have a vague suspicion that creating utils.test.js in two separate branches might introduce some kind of merge conflict.

If you rebase your branch against origin/master you should have a file now that you can add to.

Rebase without squash that is.

That didn't go as planned...
I think it's going to be simpler for me to submit another pull request, replacing this one.
Squashing into master...jc275:remove-sequential-semicolons

This throws an error if invalid CSS is found during `csstree.walk`. I couldn't find a nice way to output the line or surrounding nodes to help the dev.

Lines 132-143 are moved outside the if-block, but are otherwise unchanged.

<p>This Pull Request updates dependency <a href="https://renovatebot.com/gh/facebook/jest">jest</a> from <code>v23.4.2</code> to <code>v23.5.0</code></p> <p><details><br /> <summary>Release Notes</summary></p> <h3 id="v2350httpsgithubcomfacebookjestblobmasterchangelogmd82032350"><a href="https://renovatebot.com/gh/facebook/jest/blob/master/CHANGELOG.md#2350"><code>v23.5.0</code></a></h3> <p><a href="https://renovatebot.com/gh/facebook/jest/compare/v23.4.2…v23.5.0">Compare Source</a></p> <h5 id="features">Features</h5> <ul> <li><code>[jest-cli]</code> Add package name to <code>NotifyReporter</code> notification (<a href="https://renovatebot.com/gh/facebook/jest/pull/5898">#5898</a>)</li> <li><code>[jest-runner]</code> print stack trace when <code>process.exit</code> is called from user code (<a href="https://renovatebot.com/gh/facebook/jest/pull/6714">#6714</a>)</li> <li><code>[jest-each]</code> introduces <code>%#</code> option to add index of the test to its title (<a href="https://renovatebot.com/gh/facebook/jest/pull/6414">#6414</a>)</li> <li><code>[pretty-format]</code> Support serializing <code>DocumentFragment</code> (<a href="https://renovatebot.com/gh/facebook/jest/pull/6705">#6705</a>)</li> <li><code>[jest-validate]</code> Add <code>recursive</code> and <code>recursiveBlacklist</code> options for deep config checks (<a href="https://renovatebot.com/gh/facebook/jest/pull/6802">#6802</a>)</li> <li><code>[jest-cli]</code> Check watch plugins for key conflicts (<a href="https://renovatebot.com/gh/facebook/jest/pull/6697">#6697</a>)</li> </ul> <h5 id="fixes">Fixes</h5> <ul> <li><code>[jest-snapshot</code> Mark snapshots as obsolete when moved to an inline snapshot (<a href="https://renovatebot.com/gh/facebook/jest/pull/6773">#6773</a>)</li> <li><code>[jest-config]</code> Fix <code>--coverage</code> with <code>--findRelatedTests</code> overwriting <code>collectCoverageFrom</code> options (<a href="https://renovatebot.com/gh/facebook/jest/pull/6736">#6736</a>)</li> <li><code>[jest-config]</code> Update default config for testURL from 'about:blank' to '<a href="http://localhost">http://localhost</a>' to address latest JSDOM security warning. (<a href="https://renovatebot.com/gh/facebook/jest/pull/6792">#6792</a>)</li> <li><code>[jest-cli]</code> Fix <code>testMatch</code> not working with negations (<a href="https://renovatebot.com/gh/facebook/jest/pull/6648">#6648</a>)</li> <li><code>[jest-cli]</code> Don't report promises as open handles (<a href="https://renovatebot.com/gh/facebook/jest/pull/6716">#6716</a>)</li> <li><code>[jest-each]</code> Add timeout support to parameterised tests (<a href="https://renovatebot.com/gh/facebook/jest/pull/6660">#6660</a>)</li> <li><code>[jest-cli]</code> Improve the message when running coverage while there are no files matching global threshold (<a href="https://renovatebot.com/gh/facebook/jest/pull/6334">#6334</a>)</li> <li><code>[jest-snapshot]</code> Correctly merge property matchers with the rest of the snapshot in <code>toMatchSnapshot</code>. (<a href="https://renovatebot.com/gh/facebook/jest/pull/6528">#6528</a>)</li> <li><code>[jest-snapshot]</code> Add error messages for invalid property matchers. (<a href="https://renovatebot.com/gh/facebook/jest/pull/6528">#6528</a>)</li> <li><code>[jest-cli]</code> Show open handles from inside test files as well (<a href="https://renovatebot.com/gh/facebook/jest/pull/6263">#6263</a>)</li> <li><code>[jest-haste-map]</code> Fix a problem where creating folders ending with <code>.js</code> could cause a crash (<a href="https://renovatebot.com/gh/facebook/jest/pull/6818">#6818</a>)</li> </ul> <h5 id="chore--maintenance">Chore & Maintenance</h5> <ul> <li><code>[docs]</code> Document another option to avoid warnings with React 16 (<a href="https://renovatebot.com/gh/facebook/jest/issues/5258">#5258</a>)</li> </ul> <h5 id="chore--maintenance-1">Chore & Maintenance</h5> <ul> <li><code>[docs]</code> Add note explaining when <code>jest.setTimeout</code> should be called (<a href="https://renovatebot.com/gh/facebook/jest/pull/6817/files">#6817</a>)</li> </ul> <hr /> <p></details></p> <hr /> <p>This PR has been generated by <a href="https://renovatebot.com">Renovate Bot</a>.</p>

<p>This Pull Request updates devDependency <a href="https://renovatebot.com/gh/fastify/fastify-static">fastify-static</a> from <code>v0.12.0</code> to <code>v0.13.0</code></p> <p><details><br /> <summary>Release Notes</summary></p> <h3 id="v0130httpsgithubcomfastifyfastify-staticreleasesv0130"><a href="https://renovatebot.com/gh/fastify/fastify-static/releases/v0.13.0">v0.13.0</a></h3> <p><a href="https://renovatebot.com/gh/fastify/fastify-static/compare/v0.12.0…v0.13.0">Compare Source</a></p> <ul> <li>Updated readalbe-stream@3 <a href="https://renovatebot.com/gh/fastify/fastify-static/issues/68">#68</a> </li> <li>Dropped support for Node 9.</li> </ul> <hr /> <p></details></p> <hr /> <p>This PR has been generated by <a href="https://renovatebot.com">Renovate Bot</a>.</p>

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

I didn't like that we had extracted out some utility functions but never had *any* tests for them. The most important tests are the tests that use the public API but if the code was sufficiently complex that utility functions had to be broken out, we should have at least some tests for them.

…#255 (peterbe#256) As described in peterbe#255

jc275 · 2018-08-17T17:02:29Z

Sorry for the chaos! Closing in favour of #259. Let's try again...

As discussed in #244. All commits from that branch are merged and squashed into this one, along with a test in `utils.test.js`.

strip out double semicolons to prevent csso crash

0246c67

peterbe reviewed Aug 1, 2018

View reviewed changes

peterbe previously requested changes Aug 1, 2018

View reviewed changes

jc275 added 2 commits August 1, 2018 16:35

Create extra-semicolons.css

9f061ea

Create extra-semicolons.html

7437a18

peterbe reviewed Aug 1, 2018

View reviewed changes

jc275 added 4 commits August 1, 2018 18:23

Update extra-semicolons.css

7236a89

Add test for handling extra semicolons

9b10dc8

add comment about CSSO bug

a53de61

remove unnecessary element

b71693f

jc275 added 3 commits August 2, 2018 08:56

simplify unit test css

77f93b8

test will fail

simplify test js

a755c0d

simplify unit test html

cfd1146

stereobooster reviewed Aug 8, 2018

View reviewed changes

jc275 added 4 commits August 8, 2018 21:30

refactor: removeSequentialSemis ⇒ utils

1879681

meta: add semicolon

23da36b

refactor: removeSequentialSemis ⇒ utils

962986e

simplify semicolon unit test

e1b54e2

jc275 changed the title ~~strip out double semicolons to prevent csso crash~~ Handle multiple semicolons to prevent csso crash, fixes #243 Aug 8, 2018

stereobooster approved these changes Aug 9, 2018

View reviewed changes

peterbe requested changes Aug 16, 2018

View reviewed changes

comment on removeSequentialSemis

902d275

jc275 and others added 25 commits August 16, 2018 22:44

functional commenting

2c795aa

Throw error if invalid CSS is encountered (peterbe#237)

319279a

This throws an error if invalid CSS is found during `csstree.walk`. I couldn't find a nice way to output the line or surrounding nodes to help the dev.

update CHANGELOG for 0.7.7

6c37e26

0.7.7

ce245ac

Update dependency @types/node to v8.10.24

8583b1a

fulfilledPromise

5951a86

Lines 132-143 are moved outside the if-block, but are otherwise unchanged.

Update dependency prettier to v1.14.0 (peterbe#238)

8704af8

Update dependency prettier to v1.14.1

9017b42

Update dependency jest to v23.4.2

9b86220

Update dependency typescript to v3

c0a8db7

Update dependency fastify to v1.9.0

d819b5c

It seems we gave up idea of TS, so lets remove it

3da3a21

Add ignoreCSSErrors option

4be7232

prep for 0.7.8

7bf74cd

0.7.8

5947ea0

Update dependency prettier to v1.14.2

fb116a0

changelog for 0.7.9

03eb8b2

0.7.9

d191f58

Disregard #fragment identifier in stylesheet link.href, fixes peterbe…

c60398f

…#255 (peterbe#256) As described in peterbe#255

update changelog

c9542d7

test utils.removeSequentialSemis

468050b

jc275 mentioned this pull request Aug 17, 2018

Handle multiple semicolons to prevent csso crash, fixes #243 #259

Merged

jc275 closed this Aug 17, 2018

peterbe pushed a commit that referenced this pull request Aug 17, 2018

Handle multiple semicolons to prevent csso crash, fixes #243 (#259)

5efd93d

As discussed in #244. All commits from that branch are merged and squashed into this one, along with a test in `utils.test.js`.

jc275 deleted the patch-1 branch August 17, 2018 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle multiple semicolons to prevent csso crash, fixes #243 #244

Handle multiple semicolons to prevent csso crash, fixes #243 #244

jc275 commented Aug 1, 2018

peterbe Aug 1, 2018

peterbe left a comment

peterbe Aug 1, 2018

peterbe commented Aug 1, 2018

jc275 commented Aug 1, 2018

stereobooster Aug 8, 2018

stereobooster Aug 8, 2018

stereobooster commented Aug 8, 2018

jc275 commented Aug 8, 2018

stereobooster commented Aug 9, 2018

jc275 commented Aug 9, 2018

peterbe Aug 16, 2018

peterbe Aug 16, 2018

peterbe Aug 16, 2018

jc275 Aug 16, 2018

peterbe Aug 17, 2018

peterbe Aug 17, 2018

jc275 Aug 17, 2018

jc275 commented Aug 17, 2018

Handle multiple semicolons to prevent csso crash, fixes #243 #244

Handle multiple semicolons to prevent csso crash, fixes #243 #244

Conversation

jc275 commented Aug 1, 2018

Choose a reason for hiding this comment

peterbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbe commented Aug 1, 2018

jc275 commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stereobooster commented Aug 8, 2018

jc275 commented Aug 8, 2018

stereobooster commented Aug 9, 2018

jc275 commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jc275 commented Aug 17, 2018