Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Optimise css regeneration #789

Closed
balupton opened this Issue · 17 comments

3 participants

@balupton
Owner

from https://github.com/bevry/docpad/blob/b29a8bd04d2f53a3eef7207b6633e5fb4ad0a6be/src/lib/docpad.coffee#L1898-L1924

We should not enable referencesOthers for stylesheets
But instead check if there is a stylesheet that has been modified and is to be regenerated
Then add the rest of the stylesheets to the regenerate list
That way, when you modify a html file, it doesn't regenerate stylesheets, unless they are actually referencing others
But it would still mean that if you modify a stylsheet, it does import the others correctly
However, really, this only applies to stylsheets that concantate the contents of their @imports to other stylesheets
So maybe, we could do something like
$contains: '@import'
but $contains doesn't exist yet
though this still only applies to those that bundle other stylesheets inside themselves

@dimitarkolev

From my experience when someone is working on stylesheets is very change intensive process so having perfect understanding what is the reference graph will be great. Having $contains will improve at least 10 more scenarios i have and it will be great addition not only for imports.

@dimitarkolev

After #785 was fixed. There is a nice workaround that can improve the stylesheet regeneration process in most cases. The only thing that need to be done is to add following configuration do docpad.coffee. If you are not doing some crazy @import magic in your stylesheets it works great.

    events:
        extendCollections: (opts) ->
            @docpad.getCollection('stylesheet').on('add', (document) ->
                document.setMetaDefaults(standalone:true))           
@balupton
Owner

I'm thinking really, we could just remove this referenceOthers set on stylesheets, and make it up to the build tool plugins or users to include. As for instance, if you're just working with vanilla CSS, then there is no need for referenceOthers.

So here's my plan:

  1. Remove the referenceOthers: true set for all stylesheets within docpad
  2. Have css renderer plugins disable concatenation by updating each css renderer plugin and ensuring any concat options are off
  3. Create a concat css plugin, that sets referenceOthers: true on the concat-ified stylesheets
@balupton
Owner

@RobLoach would you like to create a docpad-plugin-webpackdocs from your docpad-plugin-webpack to be updated to like how docpad-plugin-browserifydocs plugin works, then we'll make that an official plugin, and implement steps 2 and 1 above.

@dimitarkolev

You can add a configuration for css render plugins that will enable concatenation and set referenceOthers to true. This was the user will have an option to workaround the limitation. I think another css plugin is not needed.

@balupton
Owner

Well, by having it so only a concatenation plugin has it enabled, then we can specifically target the sole file that concatenates. Alternatively, if someone wants one of their css pre-processor render files concatenated they should add the referencesOthers: true meta data to the document that they want concatenated.

Ideally, we would implement the referencesCollections: ['stylesheets'] option, or the referencesFiles: ['./blah', './blah'] ref - #336 - so that way, when you modify a html file, it doesn't regenerate your stylesheets.

@balupton
Owner

Alternatively, instead of doing a webpack plugin we could do a clean-css plugin for concat https://github.com/GoalSmashers/clean-css

Doing an enhance-css plugin too would be cool: https://github.com/GoalSmashers/enhance-css

@balupton balupton referenced this issue from a commit
@balupton balupton v6.62.0. Improvement.
- v6.62.0 January 28, 2014
	- Stylesheets are more effeciently generated
		- Files with the `outExtension: 'css'` are now the only ones included
in the `stylesheet` collection
		- Stylesheets no longer have `referencesOthers` to `true` on them by
default, this is now left up to plugin authors to do
		- Thanks to [Dimitar Kolev-Dick](https://github.com/dimitarkolev) for
[issue #789](#789)
acc930c
@balupton
Owner

We've released DocPad v6.62.0 that removes the referencesOthers by default for stylesheets.

In doing so, we've released cleancss and updated less which now set the referencesOthers: true for the appropriate css files.

@balupton balupton closed this
@dimitarkolev

I think that current solution covers only 50% of the use cases. Lets say you have less files from an external library, LessHat for example and you place those files in files folder and then @import them form default.css.less located in documents folder. In this case we will have a lot of files being regenerated because of refenceOthers flag. It will be much better to investigate the @imports and verify if the user is importing already existing file or another that is subject to generation and set referenceOther to true only in such cases. I want to vote for reopening of the issue.

P.S. From my experience having less frameworks in files and @importing them in your documents css.less is the easiest option to use them as is without changing extensions.

@balupton
Owner

Lets say you have less files from an external library, LessHat for example and you place those files in files folder and then @import them form default.css.less located in documents folder.

In this example, when a file changes, default.css.less would be the only css file regenerated, as well as other documents that include referencesOthers: true like html content listings. The other css files won't be regenerated, as they are not being rendered.

What we've done is moved out the referencesOthers: true set from docpad on all stylesheets, into plugins for only specified stylesheets — those which concatenate and import others.

@dimitarkolev

I have LessHat refered by all .css.less files which means that all of them will be regenerated as well as almost 80% of the html files (It might be only my use case but i tend to refer documents from one to another (layout based) so i can retrieve content summaries. Beside that i dont see why referenceOthers for stylesheet should affect anything else than stylesheets. The same is valid for html files and javascript files. Reference others is nice thing to have but it should be used very carefully otherwise it leads to unnecessary regeneration and bad performance.

@RobLoach
Owner

Would this work?

src/documents/style.css.less

@import (less) '../files/lesshat/otherstyle.less';

If you use raw, then:

@import (less) '../raw/lesshat/otherstyle.less';

Something along those lines? Using (less) will force the LESS compiler to import it and compile it with LESS. It may mess up the CSS paths. Hmmm.

@dimitarkolev

It works even without (less) just @import less file and it works fine. But having the @import will set reference others to true and this will force regeneration of other documents that has nothing to do with stylesheets.

@balupton
Owner

Not quite sure what the issues is...

Reference others is nice thing to have but it should be used very carefully otherwise it leads to unnecessary regeneration and bad performance.

This is exactly why this issue and DocPad v6.62.0 no longer sets referencesOthers: true on all stylesheets, but only those that concatenate and import.

To stop edits of css files regenerating say html files with referencesOthers: true, or a html file regenerating css files with referencesOthers: true, #336 must be implemented. If that is what is what you are after, then #336 seems to be the place to discuss.

@RobLoach
Owner

@dimitarkolev Are you saying it should referenceOthers less than what it currently does? referenceOthers is currently toggled true only when the stylesheet an @import in it. Are you saying it should do it even less and figure out which files are imported so that it knows which children should be updated?

@dimitarkolev

@RobLoach yes i am saying that referenceOther should be set to true even less often. And less plugin should ivestigate the @import clause. If the import is refering document it should be set to true. If the import if refering file from raw or files referenceOther should stay false.

@dimitarkolev

@balupton I agree that #336 will solve many problems but it is very complex approach that will have its own drawbacks. I am discussing an improvement to the current solution - less plugin behavior to be exact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.