remove generation bottleneck; add custom postprocessing #15
Conversation
There was a problem hiding this comment.
Thanks for the PR. I added a couple of comments for specific bits of code.
More generally, I'm wondering if a better solution might be to keep wiping out _site and only add the post-build step as you have, but run the post-build commands after Elmstatic populates _site. So in your case, the post-build step would be to regenerate all the PostCSS output. The reason for this is that your current solution doesn't appear to handle the scenario where source files are deleted or renamed.
| return Fs.mkdirs(Path.dirname(outputPath)) | ||
| .then(() => { | ||
| return Fs.writeFile(outputPath, page.html) | ||
| }) |
There was a problem hiding this comment.
I'm not keen to add the complexity of async functions unless there is a tangible performance improvement – I've deliberately stuck to sync functions so far in order to keep the code simple. My impression was that the run time is dominated by running JS. Do you have performance numbers for this?
There was a problem hiding this comment.
time elmstatic build
first run
latest elmstatic:
real 0m4.088s
user 0m1.639s
sys 0m0.813s
@eimfach/elmstatic
real 0m2.603s
user 0m1.359s
sys 0m0.480s
second run
latest elmstatic
real 0m2.293s
user 0m1.381s
sys 0m0.511s
@eimfach/elmstatic
real 0m1.308s
user 0m1.112s
sys 0m0.325s
There was a problem hiding this comment.
I am using the elmstatic boilerplate, I thought you could test the performance locally on korban.net ? npm i @eimfach/elmstatic
There was a problem hiding this comment.
writing sync is always blocking the main execution. You also could generate the html in the first place and then write sync, but not html -> write sync -> html -> write sync ... and so on
| if (options.keepOutputAlive) { | ||
| ; // do nothing | ||
| } else { | ||
| log(` Cleaning out the output path (${config.outputDir})`) | ||
| Fs.emptyDirSync(config.outputDir) | ||
| } | ||
|
|
There was a problem hiding this comment.
Looks like this is going to be problematic if a post/page is deleted or renamed. What was your plan here?
I'd also prefer to avoid adding configuration options if possible. This seems like the kind of change that should "just work" for everybody.
There was a problem hiding this comment.
Hm I see two possible solutions here
-
Be pragmatic, since it is a 'development build' served by elmstatic watch and just keep the deleted and 'previous named' files.
-
Or be specific about the events, and add handles for 'delete' and 'rename' events from chokidar...
There was a problem hiding this comment.
Deleting everything is a problem, it leads to unconsitency in conjunction with postcss or other postprocessing in the reloads when you use for example 'live-server' or 'browser-sync'
There was a problem hiding this comment.
Also the new config entry is passed by default to 'generateEverything' in watch mode...
I am not sure if keepOutputAlive is even a new config entry in this case ? It is just passed in the options param...
bin/elmstatic.js
Outdated
| log(" Generating feeds") | ||
| generateFeeds(config.feed, config.outputDir, R.reject(R.propEq("isIndex", true), newPosts)) | ||
|
|
||
| if (config.postProcess.length > 0) { |
There was a problem hiding this comment.
I think the best way to check this would be !R.isNil(config.postProcess) && !R.isEmpty(config.postProcess).
There was a problem hiding this comment.
I will change it accordingly, but I also realized this config entry is not needed anymore, if the output dir is not deleted ...
I think the first option is problematic. For example, when using GitHub Pages, the build output is committed, so deleted pages/posts will remain accessible on the live site. The second option adds quite a bit of complexity to the implementation. On reflection, I'm not sure that this is a direction I'd like to go in. My preference would be for people to use I'll revisit the async changes, but I think I'll do it after I add support for Elm 0.19.1, as I consider it to be higher priority. |
|
Using worker threads for improved performance now. |
#14 I changed the file writings in the html generation process to be async, this should speed up things a lot...(if not too much, then spawning child processes could be an option). I also created a new config entry where users can add their own post processing commands. Also watch mode does not clean up the output directory anymore, which is I hopefully think, ok.