Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronous for+include generate asynchronous code instead of synchronous #527

Closed
tdelmas opened this issue Sep 21, 2015 · 5 comments
Closed

Comments

@tdelmas
Copy link

tdelmas commented Sep 21, 2015

According to the documentation :

https://mozilla.github.io/nunjucks/templating.html#asynceach

asyncEach is an asynchronous version of for. You only need this if you are using a custom template loader that is asynchronous;

Before, the compilation of

{% for item in items %}
  {% include "item-template.html" %}
{% endfor %}

The compilation generate synchronous calls but after that commit :
7d4716f

the same code is considered as asynchronous.

According to the documentation, the writer of the template is supposed to decide between for and asyncEach but according to that commit, to compiler for the code to be asynchronous.

As the recursion limits of Firefox and safari can be as low as 500, it became really easy to hit these limits.

Am I missing something ?

@carljm
Copy link
Contributor

carljm commented Sep 21, 2015

@jlongster Do you have any comment on this? I'm not really familiar with the async stuff at all.

@carljm
Copy link
Contributor

carljm commented Dec 14, 2015

@tdelmas The commit you linked certainly seems wrong to me. Simply using an include should not automatically trigger async, unless you've explicitly asked for an async loop.

Unless @jlongster weighs in to defend that commit, I will plan to revert it.

@nicolasartman
Copy link

@carljm Anything blocking the revert? I just ran into this as it made it impossible to catch errors in includes, so I would love to see this fixed.

@carljm
Copy link
Contributor

carljm commented Feb 16, 2016

I think I was hoping to get more of an explanation from @jlongster before reverting, but since that doesn't seem to be forthcoming, I'll go ahead and revert it today.

@carljm carljm closed this as completed in 525abc2 Feb 16, 2016
carljm added a commit that referenced this issue Feb 16, 2016
…r statements (fixes #372)"

This reverts commit 7d4716f.

Using an include is not (in and of itself) a request to turn a for-loop
async. It may be that some version of this fix is needed, but if so it needs to
be more carefully targeted.

Fixes #527; re-opens #372.
@nicolasartman
Copy link

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants