-
Notifications
You must be signed in to change notification settings - Fork 170
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
[Discussion] Handling variables in deferred scopes #421
Conversation
I think the only way you can render this correctly is you store a reference to the current context with the deferred node. Even safer would be be if you stored a copy of the context (and all parents!) at the time at that the node was deferred. I think that might be very slow and consume a lot of memory though. |
@boulter I think that would solve the var being undefined within the deferred context and I think storing it with the deferred node might be one of the better options. Do you have any thoughts about the additional case here where we would like to mark a variable used inside and outside the deferred score as deferred when used outside. I was playing around with a way to continue parsing the node that threw the deferred exception and then pull out the tokens and popping the interpreter stack at the end. However I didn't see an easy way to pull the subject out a set expression for example. |
I'm not sure I'm getting this. Could you expand your example above with expected values for each case? |
I've updated the main body with an example run through of what should happen. Let me know if any part needs more detail |
I went through and added some icons to try to keep the correct and incorrect values straight. Did I get them right? Why are What is the Assuming I understand the problem, I'm not sure I have a great solution for it. I think it will just require some tricky logic and accounting in in jinjava. All the deferred stuff was introduced by Jannik and Padraig and I'm not sure that they considered this complication. |
Yes the for loop is to get another context. Its more straight forward than an if because there is different logic for handling if's that try to evaluate the conditional so I thought a loop with a single item would be easier to repro. By your annotations I'm assuming we are saying correct is desired but not necessarily functionally correct for the current behaviour and incorrect is undesired but potentially technically correct. Let me know if you had a different meaning. Proposed logic:1st Render:✅ varUsedInForScope=outside if statement : incorrect as we want this to be marked as Deferred with an original value of outside if statement ❌ deferred=com.hubspot.jinjava.interpret.DeferredValue : correct because this is the first pass and we have not resolved the variable yet before the 2nd pass. The loop and item variables are in the context in that example because using the proposed solution of copying the context from the inner scope to the outer scope brings the loop scope into the parent. I agree with you that its going to be tricky. From what I can remember about compiler design this would change the specification of the the compiler fundamentally from a single pass to a multi pass. One solution we could consider is deferring the entire template in certain cases as how is done for macro imports. I'm worried that this could introduce significant performance penalties when rendering for 1000s of emails. There may be a fine line here where we could handle certain simple cases and fall back to full deferral, evaluating the performance at a later date. From talking to Jannik about it, we did consider the need to defer lower scopes, along with some "look ahead" behaviour. We figured it would be something we would solve at a later date in the deferred data project (which allows us to introduce better HubL support in email). But it turns out this becomes important in handling HubSpot modules used in DnD. Thanks for your time helping me to thrash this out. |
Resolved by #438 |
I'm working on preserving variables within deferred blocks. Currently, if a variable is used inside of a deferred node then once the first render is finished, the value is no longer in the context at the end of the render. This also presents itself as a bug in our (HubSpot) implementation when using Modules as the modules are rendered in a lower scope.
This POC PR shows that by taking the context at the time of encountering the deferred node and adding it to the parent context we can preserve it for the subsequent render. However, there is an issue with this. If the variable is used in the deferred block and updated, the first pass will resolve the variable with the value ignoring the deferred logic.
Ideally, when encountering a deferred block, we could look at the variables used in that block and add them to the context as a DeferredValue with an original value of the value they have at the time of deferral. This requires continuing to evaluate the node after the exception has been thrown, which is tricky.
Example template:
Context before rendering:
resolved=resolvedValue
deferred=com.hubspot.jinjava.interpret.DeferredValue
Output when rendering with existing logic:
❌
{% if deferred %} {{ varUsedInForScope }} {% set varUsedInForScope = 'entered if statement' %} {% endif %} outside if statement
Context after 1st render:
❌ varUsedInForScope is lost
✅ deferred=com.hubspot.jinjava.interpret.DeferredValue
✅ resolved=resolvedValue
Before the 2nd render we set the deferred var to have a non-deferred value to allow the if block to execute.
Output 2nd render:
❌
outside if statement
Note the missing first print statement from inside the deferred statement. The second printed statement is simply preserved from the 1st render and should have the value "entered if statement"
Context after 2nd render:
✅ varUsedInForScope=entered if statement
✅ deferred=resolved
✅ resolved=resolvedValue
Output when rendering with proposed logic:
Context before render:
resolved=resolvedValue
deferred=com.hubspot.jinjava.interpret.DeferredValue
❌
{% if deferred %} {{ varUsedInForScope }} {% set varUsedInForScope = 'entered if statement' %} {% endif %} outside if statement
Context after first render:
✅ loop=com.hubspot.jinjava.util.ForLoop
✅ item=resolvedValue
❌ varUsedInForScope=outside if statement : we want this to be marked as Deferred with an original value of outside if statement
✅ deferred=com.hubspot.jinjava.interpret.DeferredValue : correct because this is the first pass and we have not resolved the variable yet before the 2nd pass.
✅ resolved=resolvedValue
Again, deferred is set to a resolved value before the 2nd render
Output 2nd render:
❌
outside if statement outside if statement
Context after 2nd render:
✅ loop=com.hubspot.jinjava.util.ForLoop
✅ item=resolvedValue
❌ varUsedInForScope=entered if statement
✅ deferred=resolved
✅ resolved=resolvedValue
This result is more functional as it allows the var in the if block to be evaluated but it does not have the expected new value for varUsedInForScope
Desired output
We would like the output of the example template to be the same as if there was only one render stage with the deferred value present and available in the 1st stage.
Example template:
The desired output for the example template would be:
outside if statement entered if statement
In order to achieve this we want the following to occur:
Context before rendering:
resolved=resolvedValue
deferred=com.hubspot.jinjava.interpret.DeferredValue
Output of 1st render:
✅
{% if deferred %} {{ varUsedInForScope }} {% set varUsedInForScope = 'entered if statement' %} {% endif %} {{ varUsedInForScope }}
The for loop is rendered away with just one iteration occurring as desired. The first rendering stage would mark varUsedInForScope as deferred and store its original value - preventing it from being rendered with the current value in the context. This requires parsing ahead within the deferred block to know that it is used in the deferred if statement.
Context after first render:
✅ varUsedInForScope=com.hubspot.jinjava.interpret.DeferredValue(originalValue=outside if statement)
✅ deferred=com.hubspot.jinjava.interpret.DeferredValue
✅ resolved=resolvedValue
Output of 2nd render:
✅
outside if statement entered if statement
As desired.