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

Fix issue where sync render would not raise errors in included templates #1301

Merged
merged 1 commit into from
Jul 19, 2020

Conversation

fdintino
Copy link
Collaborator

@fdintino fdintino commented Jul 18, 2020

Summary

The fix introduced in PR #1049, preventing multiple calls to the render callback, was a bit too aggressive and prevented some template errors from being surfaced when render or renderString were called synchronously.

The real bug here lies in the compiler, which should not be calling the same callback function more than once. But the way that waterfall tasks are chained makes it difficult to properly halt execution in a template. We ought to fix the underlying issue, but for now this should (hopefully) ensure that thrown errors always surface when rendering synchronously, while not causing a regression for #1029.

Closes #1272 . I suspect this also fixes #1246, but I can't confirm because the reporter of that issue hasn't provided a test case I can use.

Checklist

  • Proposed change helps towards purpose of this project.
  • Tests are added / updated to cover proposed change.
  • Changelog has an entry for proposed change (if user-facing fix or feature).
  • Documentation is added / updated to describe proposed change. (N/A)

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2020

Codecov Report

Merging #1301 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1301      +/-   ##
==========================================
- Coverage   89.61%   89.60%   -0.01%     
==========================================
  Files          22       22              
  Lines        3043     3041       -2     
==========================================
- Hits         2727     2725       -2     
  Misses        316      316              
Impacted Files Coverage Δ
nunjucks/src/environment.js 85.82% <100.00%> (-0.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63c4baf...bbcbaf3. Read the comment docs.

// tasks are both not passing errors up the chain of callbacks AND are not
// causing a return from the top-most render function). But fixing that
// will require a more substantial change to the compiler.
if (didError && cb && typeof res !== 'undefined') {
Copy link
Collaborator Author

@fdintino fdintino Jul 18, 2020

Choose a reason for hiding this comment

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

The second call to the root render callback in issue ##1029 happened here in compileRoot:

this._emitLine('if(parentTemplate) {');
this._emitLine('parentTemplate.rootRenderFunc(env, context, frame, runtime, cb);');
this._emitLine('} else {');
this._emitLine(`cb(null, ${this.buffer});`);
this._emitLine('}');

If an error occurs inside an included template, the error will be passed up the callback chain, and the user's callback will get called with the correct error argument. But it won't cause a return from the root compiled template function, so it will still call cb(null, this.buffer) at the very end. this.buffer is always a string, even if empty, so by confirming that res is not undefined we can check specifically for the situation described above.

@fdintino fdintino merged commit bbcbaf3 into master Jul 19, 2020
@fdintino fdintino deleted the fix-swallowed-included-template-errors branch July 19, 2020 22:55
muster-mark added a commit to muster-mark/mark-fisher-photography that referenced this pull request Aug 13, 2020
muster-mark added a commit to muster-mark/mark-fisher-photography that referenced this pull request May 1, 2021
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.

Exceptions being swallowed after import errors in included templates
2 participants