Refactor rendering handling

ArmorDarks commented 6 years ago

Hi

I've encountered issues while building async filter when using grunt-nunjucks-2-html. Turned out, they are related to the way how we use Promises.

I've already noticed before some quirks with wrong messages like 4/5 files rendered, but not producing any actual errors. I could never figure out where they were coming from.

Finally, figured out how to fix it.

Fixed issue with race conditions caused by templates with async filters (and most likely with async loaders).

Task finished before async templates actually were rendered.
Improved error handling

The task will properly count and display successfully rendered and errored templates and fail if there were any errors.

Note, that right now task will try to render all templates, even with errors, and fail task only in the end if there were any errors during rendering. It was intentional behavior since it allows to see all errors of templates during a single run, though it might be annoying in large projects. Maybe we need to handle it with an option.
Simplified code

This is PR for 2.x branch, but if it's ok it can be easily adapted for master branch.

ArmorDarks commented 6 years ago

The code could be improved even further with destructuring, but we need to bump Node version to 6.0.0 then.

ArmorDarks commented 6 years ago

It would be good to add tests for proper console output and error handling, as well as async templates, rendering (the ones with async filters), but with the current tests set up, it would be tricky. Anyway, it isn't related to this PR.

Also, feel free to adjust code style to your liking. Hopefully, now we've got proper general approach with Promise.all, all other things just a matter of taste. Personally, I just need to be sure that it works correctly with async templates :)

ArmorDarks commented 6 years ago

And totally forgot — how console outputs look with this PR:

wo8khid

vitkarpov commented 6 years ago

@ArmorDarks thanks for you work!

It would be good to add tests for proper console output and error handling, as well as async templates, rendering (the ones with async filters), but with the current tests set up, it would be tricky.

What exactly is tricky? I think we should add tests within this pr to be sure the next changes don't break anything

ArmorDarks commented 6 years ago

What exactly is tricky?

Because I'm not sure that current tests set up can handle promises, and I'm not familiar with the solution used for running tests.

I have a feeling that it would be easier to replace current solution with more common those days Jest.

think we should add tests within this pr to be sure the next changes don't break anything

Current PR definitely doesn't break already existing tests, which are quite ok, except that they do not test async templates.

Would you have time to add any tests, or I need to handle it too?

ArmorDarks commented 6 years ago

Though, wait... with the current solution we just need to add few additional task targets with preconfigured async filters, that should do the trick. I'll do that.

ArmorDarks commented 6 years ago

That should cover basic stuff.

I'd add some better testing, like for error messages in various scenarios, but it will require revision of how we're triggering Gruntfile building. It has to be done programmatically within test files, otherwise, we won't be able to catch errors.

Something like this and this.

But maybe we should make it a part of standalone PR? Current tests should cover basic stuff, hopefully, we're not breaking anything for anyone.

vitkarpov commented 6 years ago

Seems async filters work fine, thanks for tests.

vitkarpov commented 6 years ago

Can we make history a little bit clear? There's a couple of problems solved here:

async filters (fix with promises) + tests
refactoring

Let's create a proper git history for 👆 I mean https://github.com/vitkarpov/grunt-nunjucks-2-html/pull/56/commits/86d168138149ee14504d3c9c6e41ce258597001c could be split into two commits.

ArmorDarks commented 6 years ago

Hm, can you be more specific how do you see the splitting of https://github.com/vitkarpov/grunt-nunjucks-2-html/commit/86d168138149ee14504d3c9c6e41ce258597001c?

I'm not sure it will be possible because fixing of async handling required a complete revision of most part of the task, and it was hard to split it into stages. That's why I've committed it as a single large commit, with a detailed description.

ArmorDarks commented 6 years ago

@vitkarpov ping

vitkarpov commented 6 years ago

Hm, can you be more specific how do you see the splitting of 86d1681?

There're 2 different problems here: refactor and fix a bug with building async filter. I believe different problems should be separated into different commits.

Anyway, I'm going to make a big refactoring, clean a little, etc, so it doesn't matter right now. I'm merging this.

vitkarpov commented 6 years ago

@ArmorDarks v2.2.0 🎉

ArmorDarks commented 6 years ago

Great, thanks! Yeap, cleanup will be welcome :)

Should I make PR for master branch too, or you will cherry pick commits there?

There're 2 different problems here: refactor and fix a bug with building async filter. I believe different problems should be separated into different commits.

I agree but this is a case where fixing a bug was impossible without refactoring.

vitkarpov commented 6 years ago

If you don't need v1.x, I'll take care about it later.

пт, 23 марта 2018 г. в 21:02, Serj Lavrin notifications@github.com:

Great, thanks! Yeap, cleanup will be welcome :)

Should I make PR for master branch too, or you will cherry pick commits there?

There're 2 different problems here: refactor and fix a bug with building async filter. I believe different problems should be separated into different commits.

I agree but this is a case where fixing a bug was impossible without refactoring.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/vitkarpov/grunt-nunjucks-2-html/pull/56#issuecomment-375752453, or mute the thread https://github.com/notifications/unsubscribe-auth/AC3Nv_d7pPFdcwhApMCfVlERGgn_aoM3ks5thTizgaJpZM4Skxd4 .

ArmorDarks commented 6 years ago

btw, any plans to make NPM publish?

vitkarpov commented 6 years ago

👆 v2.2.0 is out there, made it back then.

ArmorDarks commented 6 years ago

Hm, strangely I didn't receive Greenkeeper notification. My bad, sorry.

ArmorDarks commented 5 years ago

@vitkarpov I've noticed that his PR has never made it into the master branch, so people using Nunjucks 3.x are still affected by quirky results when using async globals and filters.

If you don't intend to refactor it, it'd be better to release it as it is to the master. Code improvements can be made later anytime.

vitkarpov commented 5 years ago

Hey @ArmorDarks, thanks for noticing that! I've created a ticket for that https://github.com/vitkarpov/grunt-nunjucks-2-html/issues/57

Will try to do this when I have some time but feel free to take it if you want to contribute, you're very welcome! 👍

vitkarpov / grunt-nunjucks-2-html

Refactor rendering handling #56