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

always evaluate block.super even if it is not used #267

Merged
merged 2 commits into from
Jul 28, 2022
Merged

Conversation

ilyapuchka
Copy link
Collaborator

@ilyapuchka ilyapuchka commented Jan 21, 2019

Resolves #266

The issue is caused by the fact that we were trying to avoid rendering block.super when it is not used in a child template by searching for such variable inside block node. The problem is that if inside this node there are other nodes, i.e. {% if %} then we are not searching inside them. We could potentially search for it recursively, but this can lead to unneeded code and runtime complexity. Instead we can always render it when rendering extend node.

This will come at a cost of possibly unneeded rendering of the block (which still may be cheeper than going through nodes tree searching for it) and any errors in the base block being reported for {% extends %} node (see changed test case) instead of {{ block.super }}.

Searching for block.super would not work reliably for error reporting any way because we are searching only for first occasion of block.super node, where at runtime actual rendering may be happening in another place (i.e. in a different branch of if node).

Alternatively we could render the content of block.super lazily by storing closure instead of rendered value in the context, but quick change like that resulted in infinite recursion and probably requires more changes to make it work.

Copy link
Contributor

@djbe djbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wary of the incurred cost of always rendering. We have some users that use Stencil for quite big templates (see the slow parsing issue from a while ago), and I wouldn't want to have some massive regression because of this.

Do you have a branch with the recursion you mentioned? Might be worth taking another look.

@ilyapuchka
Copy link
Collaborator Author

@djbe I don't think I have it

@djbe djbe force-pushed the fix-block-super branch from 68a791c to 8e890db Compare July 28, 2022 00:33
@SwiftGen-Eve
Copy link

Hey 👋 I'm Eve, the friendly bot watching over Stencil 🤖

Thanks a lot for your contribution!


Seems like everything is in order 👍 You did a good job here! 🤝

Generated by 🚫 Danger

@djbe djbe added this to the 0.15.0 milestone Jul 28, 2022
Copy link
Contributor

@djbe djbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played around with lazy evaluation (see #324), but kept hitting generation order issues. I presume it's because the BlockContext is also modified during renders, causing issues with lazy-rendering.

These changes are good as is (after rebase). Yes there'll be a performance impact, but it'll fix an error, so it's worth it.

@djbe djbe merged commit 2035101 into master Jul 28, 2022
@djbe djbe deleted the fix-block-super branch July 28, 2022 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blocks and if don't seem to mix
3 participants