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

Uncss causes infinite loop #319

Closed
oleersoy opened this issue Jul 13, 2017 · 20 comments
Closed

Uncss causes infinite loop #319

oleersoy opened this issue Jul 13, 2017 · 20 comments

Comments

@oleersoy
Copy link

I'm running uncss against a fairly large CSS file (Contains all google fonts). To see the infinite loop checkout this repository:

git checkout https://github.com/superflycss/utilities-fonts
cd utilities-fonts
npm run deploy

This will end up running the superflycss deploy task. If uncss is removed the task completes. To remove do

atom node_modules/@superflycss/task-deploy/src/main/js/index.js

Change the uncss_test_processors postcss plugins array to this:

var uncss_test_processors = [pc_import, pc_each, pc_for, pc_custom_properties, pc_apply, pc_calc, pc_color_function, pc_sass_color_functions, pc_custom_media, pc_font_magician, autoprefixer, pc_reporter({
  clearMessages: true
})];

Then run the task again:

npm run deploy

It now completes in about 30 seconds.

@RyanZim
Copy link
Member

RyanZim commented Oct 3, 2017

Please post a minimal, reduced test case that includes only the code and dependencies needed to replicate this problem.

@oleersoy
Copy link
Author

oleersoy commented Oct 4, 2017

I think it might depend on the size of the CSS file. PostCSS font magician generates a lot of CSS and the tests essentially include all of it, so the quantity of CSS being processed is very large. If you run the deploy task do you see the infinite loop? Sometimes Node has issues with large data sets, so it could be that it is a node issue. I have a feeling that if I reduce the size of the test file, the loop problem will go away ... as I don't have this issue in other modules that use superflycss/font-utilties module.

@RyanZim
Copy link
Member

RyanZim commented Oct 4, 2017

I'll be very frank with you. I do not have the time or resources to read all the code in your repositories to check if there's anything malicious; and I cannot take the risk of running untrusted code from anyone's repo without reviewing it first. Therefore, I cannot debug your issue.

@oleersoy
Copy link
Author

oleersoy commented Oct 4, 2017

@RyanZim I'll be frank with you. You are probably one of the most awesome and brilliant people running / supporting amazing projects like uncss on github, and you have already helped me out a ton, so no worries at all. I will note though that unlike Bootstrap and other CSS repositories I have purposely split components, utilities, and tasks into their own repository such that reviewing the files in a repository should take no more than a few minutes. I'm also not expecting you to debug it. I'm just curious if you see the issue. If you see it at least we know that there is an issue. The HTML file that uncss is targeting is very large in this case, so it's probably something that uncss will rarely run into, but I figured I'd report it, so at least we have awareness if anyone else sees it.

@oleersoy
Copy link
Author

This is a visual demo of what uncss is running against. You'll notice even Chrome has a hard time with the rendering of all of it (Even after splitting up all the google fonts by font weight), so it could be that this is simply pushing the limits of what node/postcss/uncss is capable of:
https://superflycss.github.io/utilities-fonts/target/test/html/fw600.html

@oleersoy
Copy link
Author

Another thing I wanted to mention is that the test css file that I'm running the above tests against get really large, so uncss has a lot of work to do. The CSS utilities in general generate a lot of css, and most of the test css files import multiple repositories containing the css utility files. For example the indent utility in the @superflycss/utilities-format module looks like this:

@for $factor from 1 to 50 {
  .u-text-indent-$(factor)px {
    text-indent: $(factor)px !important;
  }

So that's 100 lines or more of css right there. So once module imports are complete for a src/test/css/index.css file like this:

@import "@superflycss/foundation";
@import "../../main/css/index.css";
@import "@superflycss/utilities-fonts";
@import "@superflycss/utilities-layout";
@import "@superflycss/utilities-colors";
@import "@superflycss/utilities-format";
@import "@superflycss/component-header";
@import "@superflycss/component-display";
@import "@superflycss/component-test";
@import "@superflycss/component-body";

There could be 60,000, perhaps even 100,000, lines of css.

@RyanZim
Copy link
Member

RyanZim commented Oct 12, 2017

Running uncss against that file just hangs uncss. I suspect the sheer size is just pushing the limits of uncss/postcss/jsdom/node as well.

@oleersoy
Copy link
Author

Indeed. I'll experiment more with this later to narrow down the exact cause. This sample has a lot of moving pieces like font rendering, etc. It would be interesting to see what would happen if I generated 100K css lines of border colors and ran it against a simple html file with one element that is using a single border style and color.

@oleersoy
Copy link
Author

@RyanZim do you know by chance if the postcss-import plugin supports relative module imports. Right now when I publish a module I set the package.json main attribute to src/main/css/index.css. This module imports all the sub modules. So if we @import "@superflycss/utilities-layout" we get every single sub module, including all the responsive ones, which might not even be needed. If instead the package.json main attribute could be set to a directory value like src/main/css and the default resolve module would be index.css, but submodules could also be resolved by doing "@superflycss/utilities-layout/flexbox" or "@superflycss/utilities-layout/margins" ... that would help cut down a lot on the total amount of css generated by some of the builds, make the build more responsive, etc. and provide more choice for users ... perhaps this is how it works already ...

BTW - If you think these types of questions are better asked on SO, just throw a shoe at me :)

@RyanZim
Copy link
Member

RyanZim commented Oct 15, 2017

You can’t set a directory in the package.json. You can do @import “ module/path/relative/to/module/root.css” however.

@oleersoy
Copy link
Author

OK - So IIUC if we declare a project dependency like this:

dependencies: 
    {  "@superflycss/utilities-layout" : "1.0.0" }

Then we can have a css file in the project that imports a submodule like this?:

@import "@superflycss/utilities-layout/src/main/css/faucet.css";

So when we want the whole kitchen sink we just do:

@import "@superflycss/utilities-layout";

Which resolves to (By means of the main attribute of the modules package.json file):
node_modules/@superflycss/utilities-layout/src/main/css/index.css

But if we only want the faucet then we can do:

@import "@superflycss/utilities-layout/src/main/css/faucet.css";

Which will then resolve to:
node_modules/@superflycss/utilities-layout/src/main/css/faucet.css

@RyanZim
Copy link
Member

RyanZim commented Oct 16, 2017

Yep, that's 100% correct.

@oleersoy
Copy link
Author

In order to shorten import strings, and minimize the length of the main attribute on package.json, do you think postcss-import could detect whether the main attribute on package.json is a directory, and if it is automatically append index.css to load the default module? Essentially mirror the way browsers work. So for example instead of having the main attribute set like this:

"main": "src/main/css/index.css"

It could be set like this:

"main": "src/main/css/"

Now it would be interpreted as a root module path, and index.css would be the default module. If we wanted to specify sub modules we could import them like this:

@import "@superflycss/utilities-layout/faucet.css";

Submodules:

@import "@superflycss/utilities-layout/valves/shutoff.css";

This would end up resolving node_modules/@superflycss/utilities-layout/src/main/css/valves/shutoff.css.

@RyanZim
Copy link
Member

RyanZim commented Oct 16, 2017

No, I don't think so; the main field is a Node.js standard, and the style field is handled similarly by a de-facto standard. I don't want to introduce anything that's postcss-import specific.

@oleersoy
Copy link
Author

oleersoy commented Oct 17, 2017

The semantics for all the users of the main and style fields for all current packages and users stays the same. This capability is only invoked when an author of a new package sets the main field to a directory. For example if someone wants to set main or style to src/main/css/index.css they can do so. If this is implemented then they can set main or style to src/main/css/ (Saving the index.css part) and postcss-import will still resolve it correctly. However since the main or style attribute is a directory all submodules import strings are now shorter, and can leave off the src/main/css part.

I'd also be willing to implement and test this. I noticed a fork https://github.com/TrySound/postcss-easy-import that adds additional features. I could perhaps build one as well, or see if postcss-easy-import could handle it, but wanted to check with you first.

@oleersoy
Copy link
Author

oleersoy commented Nov 2, 2017

I tried something simple. Since the superflycss icon utility repository also causes the deploy task to spin I simply removed the font utilities (Which I originally reported this bug against) from the CSS test imports (src/test/css/index.css) and ran npm run deploy again. It now runs fine.

[18:32:50] Using gulpfile ~/Junk/@superflycss/utilities-untest/gulpfile.js
[18:32:50] Starting 'clean'...
[18:32:50] Finished 'clean' after 9.44 ms
[18:32:53] Using gulpfile ~/Junk/@superflycss/utilities-untest/gulpfile.js
[18:32:53] Starting 'build:css'...
[18:32:59] Finished 'build:css' after 6.06 s
[18:33:07] Using gulpfile ~/Junk/@superflycss/utilities-untest/gulpfile.js
[18:33:07] Starting 'test:css'...
[18:33:08] Finished 'test:css' after 320 ms
[18:33:10] Using gulpfile ~/Junk/@superflycss/utilities-untest/gulpfile.js
[18:33:10] Starting 'deploy:test:css'...
[18:35:18] Finished 'deploy:test:css' after 2.13 min
[18:35:28] Using gulpfile ~/Junk/@superflycss/utilities-untest/gulpfile.js
[18:35:29] Starting 'deploy:test:html'...
[18:35:29] Finished 'deploy:test:html' after 26 ms

So that does not necessarily prove that it is the font css that is causing the issue, but now I'm going to generate a lot of CSS in the test css file (Like 500K color css not used by any of the markup), and we'll see if uncss has an issue. If not then it's something about fonts that uncss does not like.

@oleersoy
Copy link
Author

oleersoy commented Nov 3, 2017

Wow - It finished! It took 22 minutes but got the job done. I generated over one million lines of css by adding the following loop to src/test/css/index.css (Which gets built to target/test/css/index.css and that's what uncss is run against to produce deploy/test/css/index.css - Note that I also included the current imports so it adds up to a lot of css):

@for $x from 0 to 50 {
  @for $a from 0 to 9 {
    @for $b from 0 to 9 {
      @for $c from 0 to 9 {
        .u-background-color-$(a)$(b)$(c) {
          background-color: #$(a)$(b)$(c) !important;
        }
        .u-border-color-$(a)$(b)$(c) {
          border-color: #$(a)$(b)$(c) !important;
        }
        .u-text-color-$(a)$(b)$(c) {
          color: #$(a)$(b)$(c) !important;
        }
        .u-fill-color-$(a)$(b)$(c) {
          fill: #$(a)$(b)$(c) !important;
        }
        .u-stroke-color-$(a)$(b)$(c) {
          stroke: #$(a)$(b)$(c) !important;
        }
      }
    }
  }
}

Results

[19:09:25] Using gulpfile ~/Junk/@superflycss/utilities-untest/gulpfile.js
[19:09:25] Starting 'clean'...
[19:09:25] Finished 'clean' after 9.87 ms
[19:09:28] Using gulpfile ~/Junk/@superflycss/utilities-untest/gulpfile.js
[19:09:28] Starting 'build:css'...
[19:10:05] Finished 'build:css' after 38 s
[19:10:08] Using gulpfile ~/Junk/@superflycss/utilities-untest/gulpfile.js
[19:10:08] Starting 'test:css'...
[19:10:08] Finished 'test:css' after 318 ms
[19:10:11] Using gulpfile ~/Junk/@superflycss/utilities-untest/gulpfile.js
[19:10:11] Starting 'deploy:test:css'...
[19:31:27] Finished 'deploy:test:css' after 21 min
[19:31:29] Using gulpfile ~/Junk/@superflycss/utilities-untest/gulpfile.js
[19:31:29] Starting 'deploy:test:html'...
[19:31:29] Finished 'deploy:test:html' after 22 ms
ole@mki:~/Junk/@superflycss/utilities-untest$ wc -l src/test/css/index.css 
92 src/test/css/index.css
ole@mki:~/Junk/@superflycss/utilities-untest$ wc -l target/test/css/index.css 
1169625 target/test/css/index.css
ole@mki:~/Junk/@superflycss/utilities-untest$ wc -l deploy/test/css/index.css 
25 deploy/test/css/index.css
ole@mki:~/Junk/@superflycss/utilities-untest$ 

I'll try running running the deploy task on the font repository again now, just to see if it will finish within half an hour.

@oleersoy
Copy link
Author

oleersoy commented Nov 3, 2017

OK - It finished and fast. Interesting. It still times out on Travis, but perhaps thats because travis builds have limited resources. I kept Chrome closed because it eats 95% of my memory (Have a lot of tabs open), so that could have been part of the issue. I guess we can close this.

@oleersoy oleersoy closed this as completed Nov 3, 2017
@RyanZim
Copy link
Member

RyanZim commented Nov 3, 2017

Travis probably times out because uncss doesn't give any output while it's running, see https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

Also, Travis builds only have 3GB of memory: https://docs.travis-ci.com/user/common-build-problems/#My-build-script-is-killed-without-any-error

@oleersoy
Copy link
Author

oleersoy commented Nov 3, 2017

Good points. I think the memory allowance should be OK. I looked at my system monitor and the build does not hog that much memory - even with the one million+ line build. I think Chrome was essentially eating all of it. I also filed an issue with Travis as I can't get the travis_wait function to work right. Tried a few different permutations and still no go. Banzai Man is pretty awesome - so I'm sure he will have a brilliant fix for it.

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