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

refactor(post/tag): render tag before content #4171

Closed
wants to merge 6 commits into from

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Mar 4, 2020

What does it do?

Continuation of #3543 (comment) and #4161

Currently post rendering process is something like this: #4161 (comment). But basically, hexo will render markdown first, then the tag (using nunjucks). So hexo has to remove all swig tags before markdown rendering, and restore them after markdown rendering is finished.

In this PR, I make hexo renders tag first, then markdown, so that remove and restore swig tag are not necessary any more, resulted in 6% faster in generation speed. And the post rendering related test cases are passed as well.

I wonder why Hexo has to render tag after markdown at the beginning.

How to test

git clone -b render-tag-before-md https://github.com/sukkaw/hexo.git
cd hexo
npm install
npm test

Screenshots

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.

Issue resolved: #2821

lib/hexo/post.js Outdated Show resolved Hide resolved
@SukkaW
Copy link
Member Author

SukkaW commented Mar 4, 2020

I am considering fixing #3346 in this PR as well since that issue is also related with post rendering process.


Update:

@stevenjoezhang I have added a test case for #3346. It seems that issue has been fixed.

@stevenjoezhang
Copy link
Member

{% pullquote warning %}
Text
{% endpullquote %}
# Title 0
{% pullquote warning %}
Text
{% endpullquote %}

{% pullquote warning %}
Text
{% endpullquote %}

# Title 1

{% pullquote warning %}
Text
{% endpullquote %}

{% pullquote warning %}Text{% endpullquote %}
# Title 2
{% pullquote warning %}Text{% endpullquote %}

{% pullquote warning %}Text{% endpullquote %}

# Title 3

{% pullquote warning %}Text{% endpullquote %}

截屏2020-03-04下午5 21 23

The bug reported in #4161 was reproduced. I am confirming if this is a bug (or feature) of Nunjucks
https://github.com/mozilla/nunjucks/blob/40dfdf0ea3e6ba43c3a180eca57d935c36d188c2/nunjucks/src/parser.js#L640-L649

@seaoak
Copy link
Member

seaoak commented Mar 5, 2020

Thank you for this great patch.

I wonder why Hexo has to render tag after markdown at the beginning.

Me, too. 😉


However, since #4171 (comment) is still there, we have to find out a way to solve this.

Generally speaking, in Markdown, any block content is separated by empty line(s) each other.

And custom tags in Nunjucks is not intended for Markdown block content only.
So Nunjucks does not generate additional newline characters by default.

That is,

{% pullquote warning %}
Text
{% endpullquote %}
# Title 0
{% pullquote warning %}
Text
{% endpullquote %}

should be written as folows:

{% pullquote warning %}
Text
{% endpullquote %}

# Title 0

{% pullquote warning %}
Text
{% endpullquote %}

If we want to add a newline character automatically for Nunjucks custom tags in Hexo,
the codes in lib/extend/tag.js should be modified as follows:

diff --git a/lib/extend/tag.js b/lib/extend/tag.js
index 31f9f2aa..883fdb5f 100644
--- a/lib/extend/tag.js
+++ b/lib/extend/tag.js
@@ -77,7 +77,7 @@ class NunjucksBlock extends NunjucksTag {
   }

   run(context, args, body) {
-    return this._run(context, args, trimBody(body));
+    return this._run(context, args, trimBody(body)) + '\n';
   }
 }

@@ -111,7 +111,7 @@ class NunjucksAsyncBlock extends NunjucksBlock {
       body = () => result || '';

       this._run(context, args, trimBody(body)).then(result => {
-        callback(err, result);
+        callback(err, result && `${result}\n`);
       });
     });
   }

I'm not sure whether this behavior is desirable in Hexo.
Even so, this modification should be independent of this patch.

@SukkaW
Copy link
Member Author

SukkaW commented Mar 5, 2020

Even so, this modification should be independent of this patch.

@segayuu

IMHO, it is fine to include this patch inside this PR since it is related with post rendering issue fix.


Update:

@stevenjoezhang

I have applied the patch provided by @seaoak and it seems that #4171 (comment) has been fixed.
Related test cases is added as well.

@SukkaW SukkaW requested a review from seaoak March 5, 2020 09:31
@SukkaW SukkaW changed the title refactor(post): render tag before content refactor(post/tag): render tag before content Mar 5, 2020
@SukkaW SukkaW force-pushed the render-tag-before-md branch 2 times, most recently from b770791 to 1b0819a Compare March 5, 2020 09:43
stevenjoezhang
stevenjoezhang previously approved these changes Mar 6, 2020
'{' & '}' already escaped in backtick code filter
Comment on lines -231 to -235
const cache = [];

const escapeContent = str => `<!--${placeholder}${cache.push(str) - 1}-->`;

str = str.replace(/<pre><code.*>[\s\S]*?<\/code><\/pre>/gm, escapeContent);
Copy link
Member Author

@SukkaW SukkaW Mar 6, 2020

Choose a reason for hiding this comment

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

Hexo only uses tag.render in post.js, which is after backtick_code_filter is executed.

In backtick_code_filter, { & } are already escape, so it is fine to render with nunjucks directly.

@seaoak

This comment has been minimized.

Apply suggestions from code review by @seaoak
@SukkaW
Copy link
Member Author

SukkaW commented Mar 7, 2020

This line will be simpler because result must be a string.

@seaoak Updated.

@@ -77,7 +75,7 @@ class NunjucksBlock extends NunjucksTag {
}

run(context, args, body) {
return this._run(context, args, trimBody(body));
return this._run(context, args, trimBody(body)) + '\n';
Copy link
Member

Choose a reason for hiding this comment

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

Why add \n?

Look at the following test cases, which one is expected?

    const str = [
      'begin',
      '{% test foo bar %}',
      'test content',
      '{% endtest %}',
      'end'
    ].join('\n');
    const result = await tag.render(str);

    // there is only one `\n` before and after
    result.should.eql('begin\nfoo bar test content\nend');
    // Or should there be two `\n` at the end?
    result.should.eql('begin\nfoo bar test content\n\nend');

Copy link
Member

Choose a reason for hiding this comment

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

This extra \n is to separete a Nunjucks tag from immediate-after content.
So I expect two \n at the end.

Please refer the above comment #4171 (comment)

@stevenjoezhang
Copy link
Member

Another issue: #4087
Indented code blocks will be parsed and rendered by Nunjucks.

@SukkaW
Copy link
Member Author

SukkaW commented Mar 10, 2020

@stevenjoezhang

For indented code blocks there is no better ways. For backtick code blocks we already have a filter.


Maybe a indented code filter? Any line started with 4 spaced or \t should be wrapped in <pre><code>?

@seaoak
Copy link
Member

seaoak commented Mar 10, 2020

@SukkaW @stevenjoezhang

To escape Nunjucks tags in indented code blocks properly,
we might need a new before_post_render filter (same as backtick code blocks)

I try to make a patch of such a filter:
seaoak/hexo:feature/escape_indented_code_block_before_render
https://github.com/seaoak/hexo/tree/feature/escape_indented_code_block_before_render

With merging it, this @SukkaW 's patch resolves the issue #4087.

Do you think about this new filter?
If it is useful, I continue to try to make it. 😄

@SukkaW
Copy link
Member Author

SukkaW commented Mar 10, 2020

To escape Nunjucks tags in indented code blocks properly,
we might need a new before_post_render filter (same as backtick code blocks)

I'd rather not to do that.

Only escape { and } which is inside code block, and after code block is highlighted.

Also, you can draft a PR (without actually creating it) from your branch.

image

@SukkaW
Copy link
Member Author

SukkaW commented Mar 24, 2020

@seaoak

From #4179 we might consider it is necessary for hexo to render markdown first, right?

@seaoak
Copy link
Member

seaoak commented Mar 25, 2020

@SukkaW Yes. I think so.

@SukkaW
Copy link
Member Author

SukkaW commented Mar 25, 2020

@seaoak So we should then focus on how to make swig escaping quicker.

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.

BUG with "post_link" or "post_path" tag
4 participants