JSRocksHQ / harmonic

The next static site generator
http://harmonicjs.com/
MIT License
282 stars 26 forks source link

Rewrite unit tests to not change the process's CWD, replace Traceur with 6to5, replace Grunt with gulp #98

Closed UltCombo closed 9 years ago

UltCombo commented 9 years ago

Closes #86 Closes #87 Paving the way for #96 (allow APIs to receive a sitePath argument)

@jaydson this PR is not ready for merging yet, but feel free to take a look if you have time.

I've coded this while half asleep, so I'll perform a self-review tomorrow if I have time as well.

The main benefit of the Grunt -> gulp switch here is that gulp is imperative, providing lower-level control. I've prepared the gulpfile's watch task taking in consideration projects which take a long time to build/test (improving the test time is in my TODO list, but given Harmonic's nature, odds are the tests will still take a considerable amount of time even after being properly refactored).

More specifically, the watch task works in "batch" mode -- that is, if you edit/create/remove one or more files while a build/test is running, it will wait until the test is finished and then process all the files that changed while the test was running, thus generating an incremental build and running tests again.

If anyone has trouble reading the gulpfile, take a look at the (simplified) stream flow diagram.

TODO:

UltCombo commented 9 years ago

Even in Travis, harmonic build used to take about ~1.8s and now it took 5~6s. Thing is, it requires a rather specific scenario to trigger this slow down (as detailed in the OP), so end users shouldn't be hit by it. I'll investigate it after fixing the other more serious issues in this PR.

UltCombo commented 9 years ago

Passing the path around all functions is repetitive, but that is necessary to avoid global state.

In the future, we can switch Harmonic from a singleton to a constructor/factory pattern and pass the path when instantiating it in order to avoid this cruft.

UltCombo commented 9 years ago

The gulp build task's copy pipe should be working fine now.

There's still the harmonic build taking a long time to run in the unit tests. Weirdly enough, it looks like its run time has increased after I installed a few more dependencies -- which is rather odd, seeing as harmonic build shouldn't touch the node_modules directory. I'll investigate this issue as soon as possible.

UltCombo commented 9 years ago

Can someone please review this PR? By review I mean two things:

//cc @jaydson @leobalter

jaydson commented 9 years ago

Trying out here. The build time It's really odd: ✓ should build the Harmonic site (5152ms) ✓ should create and build a new post (5106ms)

jaydson commented 9 years ago

But it seems to be working fine.

UltCombo commented 9 years ago

Yep, I noticed it too (see this comment). I did some preliminary testing a couple days ago and the build time for the usual cases (e.g. running harmonic build) is the same as before, the slow down problem only seemed to appear when passing the new path parameter (e.g. harmonic build "/absolute/path/to/project") and the given path is not the current working directory. It is a rather weird issue, I'll investigate a bit more before merging.

Thanks @jaydson for testing. :D

UltCombo commented 9 years ago

Found the issue!

Harmonic was instantiating nunjucks.FileSystemLoader which by default watches the template directory for changes. The watcher prevented the process from exiting. Weirdly enough, passing true to the noWatch parameter did not stop nunjucks from watching the template files (possible bug? Need more testing before submitting an issue in their repo).

It is also odd how we haven't been hit by this issue before.

Anyway, I've instantiated the nunjucks.Environment using the Nunjucks simple API's nunjucks.configure passing the { watch: false } option (see https://github.com/es6rocks/harmonic/commit/331100400f586dcad1a419ef566a5b22fa9a8849), now build times are back to normal! :fireworks:

UltCombo commented 9 years ago

Alas, "back to normal" is not accurate, seeing as harmonic build was never affected. The build time was only slow when providing the new optional path argument: harmonic build /absolute/path/to/project.

jaydson commented 9 years ago

Awesome job @UltCombo !!!