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

include strangeness #372

Open
jmls opened this issue Feb 16, 2015 · 10 comments
Open

include strangeness #372

jmls opened this issue Feb 16, 2015 · 10 comments

Comments

@jmls
Copy link

jmls commented Feb 16, 2015

I have a set of model data, where 3 of the model properties as has the property ui set to true. I also have this code:

radio.html

<p>radio.html: {$ property.name $}</p>
{% asyncEach property in model.properties %}
  {% if property.ui %}
  <h4>has ui</h4>
  {% endif %}
  {% include "views/input/radio.html" %}
{% endeach %}

I get this rendered:
image

I see all of the property names, and the "has ui" for the three that have the property ui set to true.

so, now I only want the radio.html included if the ui option is set, so I move the include to within the {% if .. %}

{% asyncEach property in model.properties %}
  {% if property.ui %}
  <h4>has ui</h4>
  {% include "views/input/radio.html" %}
  {% endif %}

{% endeach %}

but all I get now is

image

so, the if is definitely working (I get the "has ui" , but the include contents are not included

am I doing something wrong, or is there a bug here ?

@jmls
Copy link
Author

jmls commented Feb 18, 2015

ping ? anyone ? This is really starting to bite me :(
thanks

@devoidfury
Copy link
Contributor

I'm not sure about the include, but you should be able to change it to a macro and solve your issue. They have better repetitive/scope handling:

radio.html

{% macro radio(property) %}
<p>radio.html: {{ property.name }}</p>
{% endmacro %}

otherfile.html

{% include "views/input/radio.html" %}
{% asyncEach property in model.properties %}
  {% if property.ui %}
  <h4>has ui</h4>
  {{ radio(property) }}
  {% endif %}
{% endeach %}

@jmls
Copy link
Author

jmls commented Feb 18, 2015

thanks for the feedback - unfortunately, this workaround won't help - I think it's a problem with async in general. I am using a template loader where the template is grabbed from a db record.

I also have several dozen templates that may be needed, each one specified by a property in the data object - and I actually don't know the names (these templates are / can be user-defined)

thanks again for the clever workaround though

@jlongster
Copy link
Contributor

Sorry, this is most likely a bug with async rendering. Async is somewhat experimental, but I doubt this is hard to fix. I'll take a look soon.

@jmls
Copy link
Author

jmls commented Feb 19, 2015

I think I've solved the problem. While looking through the parser.js source, I came across this code

parseIf: function() {
        var tag = this.peekToken();
        var node;

        if(this.skipSymbol('if') || this.skipSymbol('elif')) {
            node = new nodes.If(tag.lineno, tag.colno);
        }
        else if(this.skipSymbol('ifAsync')) {
            node = new nodes.IfAsync(tag.lineno, tag.colno);

so I thought i'd try using this mysteriously undocumented ifAsync ;) and it worked ...

{% asyncEach property in model.properties %}
  {% ifAsync property.ui %}
  <h4>has ui</h4>
  {% include "views/input/radio.html" %}
  {% endif %}

{% endeach %}

and I got the desired results

is it ok to use ifAsync - or should if get converted to ifAsync automatically ?

I also seem to have a problem with filters, i'll look into that as well

@jlongster
Copy link
Contributor

@jmls Sorry I haven't looked at this until now. It definitely should automatically be converted into ifAsync in transformer.js. To be clear, you are using an async loader?

@jlongster
Copy link
Contributor

@jmls Make sure you marked your loader as async: http://mozilla.github.io/nunjucks/api.html#asynchronous

@jlongster
Copy link
Contributor

Try it now! It should work

@carljm
Copy link
Contributor

carljm commented Dec 14, 2015

The fix here seems wrong to me on its face (using an include inside a loop is not a request to make the loop asynchronous), and causes #527, so I plan to revert it in an upcoming version. Just making a note of it here so anyone affected by this bug can look into alternate resolutions.

nicolasartman added a commit to nicolasartman/nunjucks that referenced this issue Feb 16, 2016
nicolasartman added a commit to nicolasartman/nunjucks that referenced this issue 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.
@carljm
Copy link
Contributor

carljm commented Feb 16, 2016

Reverted in 525abc2 -- re-opening this bug in case anyone wants to propose a better fix. (Perhaps an include should only trigger async-conversion for the if statement if it's enclosed within an async loop? Not sure.)

@carljm carljm reopened this Feb 16, 2016
@carljm carljm closed this as completed in 4917f42 Mar 10, 2016
carljm added a commit that referenced this issue Mar 10, 2016
* 2.x: (43 commits)
  Bump versions for dev.
  Update maintenance docs.
  Revert accidental changes to mocha.js.
  Bump version to 2.4.0.
  Update changelog.
  Merge pull request #694 from mariusbuescher/master
  Add note about include and blocks; update wrapping of templating docs.
  Merge pull request #688 from pra85/patch-1
  Rename all test templates to use .j2 extension.
  Update changelog.
  Revert "make include statements trigger async conversion inside if/for statements (fixes #372)"
  Merge pull request #672 from TrySound/yargs
  Update changelog.
  Split backported and non-backported portions of #667 in changelog.
  More acks in changelog.
  Merge pull request #668 from oyyd/cn-doc
  Don't allow included templates to write to the including scope.
  Update changelog for opts.dev fix.
  Split include tests.
  Fix references to env dev opt.
  ...
@carljm carljm reopened this Mar 10, 2016
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

4 participants