Undefined path in errors

[ArmorDarks]

Hi

There is issue with errors from Nunjucks tasks — they always shows unknown path:

Seems that error expects path as one of arguments, but it never receives it. Here is part of code from Nunjucks itself:

[vitkarpov]

Hey, how's it going? Which nunjucks' version do you use? I have just downloaded nunjucks 2.0.0 and npm test says okay.

[vitkarpov]

[vitkarpov]

Oh, now I see the error — the test is not okay

[vitkarpov]

@ArmorDarks check out 0.3.1 version of the plugin. Does it work fine for you now?

[ArmorDarks]

Thanks

Unfortunately, still unknown path:

btw, am I wrong, or it's actually throwing an warning, while should throw an error?

[vitkarpov]

It seems similar to https://github.com/mozilla/nunjucks/issues/352 — trying to use env.render instead of renderString

[vitkarpov]

https://github.com/vitkarpov/grunt-nunjucks-2-html/pull/27

[vitkarpov]

@ArmorDarks try to use v0.3.2

[ArmorDarks]

Wow, it works!

I'd never thought that issue lays in rendering method... I guess renderString should pass proper path in errors too. Would you have time to open issue in Nunjucks repository, or should I?

[ArmorDarks]

Unfortunately, seems that render and then grunt.write.file seems to be 1.5 times slower:

With renderString method:

With render method:

(around 70 templates)

So, in case issue could be fixed in Nunjucks, it would be better to return to renderString method

[vitkarpov]

Yep. I think we can try to get back renderString method and get rid of warnings at the same time

[vitkarpov]

@ArmorDarks Check out v0.3.3

[vitkarpov]

Undefined paths were 'cause of renderString method isn't sync anymore (nunjucks 2.0.0 has changed api)

[ArmorDarks]

With v0.3.3 unknown path returned

[ArmorDarks]

Also note, that from now due to latest changes in async behavior task fails on quite wrong place:

Instead of Fatal error, it should exit with grunt.warn('Something went wrong.') I think, since it's not critical error and user should have ability to use --force to skip it when needed

[vitkarpov]

Okay, let's define the problems:

• you wanna see the message of the error and other templates should be processed independent?
• unknown path is a strange nunjucks' issue which can be reproduced with renderString method but with render (v0.3.2) is okay?

[vitkarpov]

And am I right in assuming that you need to fix the error anyway (questionsBlock is undefined)?

[ArmorDarks]

you wanna see the message of the error and other templates should be processed independent?

not exactly. Let's drop that part, since it's out of scope of current issue (my bad). I will create new issue with more detailed explanation of what seems wrong to me about it.

unknown path is a strange nunjucks' issue which can be reproduced with renderString method but with render (v0.3.2) is okay?

yes. I assume this means that it's upstream issue. If you don't have time for it, I will create Issue in Nunjucks repository. Right?

And am I right in assuming that you need to fix the error anyway (questionBlock is undefined)?

nope :) it's just liberately introduced mistake — call of non existing macro — to test out current error's behaviour.

Solely purpose of that issue is to understand, why errors produces messages with unknown path instead of real paths.

[vitkarpov]

Okay. To get rid of undefined paths we need to use render instead of renderString and make an issue for nunjucks.

Can you create a related issue?

[ArmorDarks]

Okay. To get rid of undefined paths we need to use render instead of renderString and make an issue for nunjucks.

I think it's better to stick with renderString, despite that issue with undefined paths. In the end of all, it works faster, while paths not so crucial

Can you create a related issue?

Done: https://github.com/mozilla/nunjucks/issues/529

[ArmorDarks]

Hm, quite strange...

I've made some more tests (around 70 templates on real project)

With v0.3.0:

With v0.3.1:

I've wrongly concluded that render method is slower thanrenderString, but actually it isn't — it's performance exactly same (maybe a bit slower, but my tests aren't that accurate)

So, we can freely switch to render method which doesn't have performance hit and has right error messages.

What is really causing slowness is _.cloneDeep method — it has very significant performance hit on large projects.

The question is — does we really need it?

I see that Object.create doesn't clone object, but it seems that references aren't affected by other templates — so, maybe we're ok to stick with much faster Object.create?

[vitkarpov]

@ArmorDarks check out v0.3.4 to clarify the performance issue. I got rid of _.cloneDeep so it has to be faster.

If it's sluggish anyway then we should make a new issue for it and make performance tests first of all.

[vitkarpov]

I've red your message for commit about performance issue. Close this.

[ArmorDarks]

Thanks once again :+1: