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

Allow nested shortcodes with markdown bodies #2630

Closed

Conversation

ARitz-Cracker
Copy link
Contributor

This PR will allow the use of nested shortcodes with markdown bodies. For example, with the shortcodes...

  • shortcodes/div_md.html: <div>{{ body | markdown | safe }}</div>
  • shortcodes/quote.html: <quote>{{body}}</quote>

Zola will now convert the following markdown...

# Begin level 0  

{% div_md() %}

## Begin level 1

{% div_md() %}

### Begin level 2

{% quote() %} Also a quote in the nested divs {% end %}

### End level 2

{% end %}

## End level 1

{% end %}

# End level 0

...into the following HTML.

<h1 id="begin-level-0">Begin level 0</h1>
<div>
	<h2 id="begin-level-1">Begin level 1</h2>
	<div>
		<h3 id="begin-level-2">Begin level 2</h3>
		<quote>Also a quote in the nested divs</quote>
		<h3 id="end-level-2">End level 2</h3>
	</div>
	<h2 id="end-level-1">End level 1</h2>
</div>
<h1 id="end-level-0">End level 0</h1>

Fixes #515

I am assuming this is still a desired feature as the issue is still open.

Sanity check:

Code changes

  • templates::filters::MarkdownFilter and site::Site have been changed to share a single Tera instance using a RwLock. This gives the MarkdownFilter the ability to eventually call itself.
  • The shortcode parser has been changed to stop at the appropriate {% end %}, instead of the first-encountered {% end %}. (Thanks to Allow recursively nested shortcodes #1475)
  • cargo test --all was ran prior to PR submission
  • Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

  • Have you created/updated the relevant documentation page(s)?
    • As far as I'm aware, the documentation does not explicitly prohibit nested short-codes.

@OkiStuff
Copy link

OkiStuff commented Sep 5, 2024

This looks awesome mate 👍

Copy link
Collaborator

@Keats Keats left a comment

Choose a reason for hiding this comment

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

Can you add a test using the nth variable of shortcodes and nesting a shortcode in itself?

@ARitz-Cracker
Copy link
Contributor Author

Thanks for taking a look at this!
So,

{{ a() }}

{{ a() }}

{% render_md() %}

{{ a() }}

{{ a() }}

{% render_md() %}

{{ a() }}

{{ a() }}

{% end %}

{% end %}

Currently outputs

<p>a: 1</p><p>a: 2</p><div><p>a: 1</p><p>a: 2</p><div><p>a: 1</p><p>a: 2</p></div></div>

But am I correct to assume you'd rather have something like this?

<p>a: 1</p><p>a: 2</p><div><p>a: 3</p><p>a: 4</p><div><p>a: 5</p><p>a: 6</p></div></div>

Also, what do you mean by "nesting a shortcode in itself"? You still cannot define a shortcode that calls itself in its own body with this PR, and I can't think of a way to create a circular shortcode with these changes off the top of my head.

@Keats
Copy link
Collaborator

Keats commented Sep 19, 2024

But am I correct to assume you'd rather have something like this?

Yes it would make more sense imo.

Also, what do you mean by "nesting a shortcode in itself"?

Kind of what you have in your example:

{% render_md() %}

{% render_md() %}

{% end %}

{% end %}

And make sure nth=1 for the outer one and nth=2 for the inner one

* Have the markdown filter and site share a common ShortcodeInvocationCounter which is reset every page or section render
* Fix typos with RwLock poison errors.
* Have MarkdownFilter::filter use RenderContext::new
* Make RenderContext::new more robust
@ARitz-Cracker
Copy link
Contributor Author

Hey, I have implemented a ShortcodeInvocationCounter that's shared between the Site and MarkdownFilter. Page and Context reset the ShortcodeInvocationCounter when they render the markdown on the top-level.

Additionally, RenderContext::new has been modified so that it is usable by MarkdownFilter::filter. Since we're on the path of sharing state anyway, we could make the markdown filter i18n aware if desired.

The nested shortcode invocations now have their nth value incremented as such:

  • 1
  • 2
    • 3
    • 4
      • 5
      • 6

That said, since all shortcodes are parsed at one level before going in the inner level, if there are nth values shown above and below the nested invocation, it goes like this:

  • 1
  • 2
    • 5
    • 6
      • 9
      • 10
    • 7
    • 8
  • 3
  • 4

Side-notes:

  • RenderContext::from_config is now unused.
  • Something similar to the ShortcodeInvocationCounter could be used to make the MarkdownFilter i18n aware if desired.

@ARitz-Cracker
Copy link
Contributor Author

Hey @Keats, is there anything else you'd like from my end?

Copy link
Collaborator

@Keats Keats 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 not sure about putting RWLock everywhere. Can we just ignore the markdown filter?

@@ -212,14 +212,17 @@ impl Page {
config: &Config,
anchor_insert: InsertAnchor,
shortcode_definitions: &HashMap<String, ShortcodeDefinition>,
shortcode_invoke_counter: &ShortcodeInvocationCounter,
Copy link
Collaborator

Choose a reason for hiding this comment

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

that function is becoming unwieldly. Not an actionable thing, just noticing it

@ARitz-Cracker
Copy link
Contributor Author

ARitz-Cracker commented Oct 3, 2024

I'm not sure about putting RWLock everywhere. Can we just ignore the markdown filter?

As far as I'm aware, partially due to the work put in place by #1358, shortcode execution within shortcodes is only possible with the markdown filter. Additionally, my personal use cases for this this PR is defining columns + rows, nested containers with different styles, etc.

There are 2 main benefits to the use of RwLock as seen here.

  1. Having the markdown filter share the same instance of Tera as the Site avoids unnecessarily cloning and saves on memory. Additionally, it allows to have the Tera instance in the markdown filter have the same filters registered, which includes itself.
  2. There's restrictions on how data flows from Zola -> Tera -> Zola, and generally, I prefer sharing data over (de)serialization or cloning when possible, so a shared reference (which if mutable Rust requires to be something like a RwLock) is used.
  • Sidenote: Arc could be used on its own, though I can't figure out off the top of my head how that could be ergonomic with zola serve.

There also doesn't seem to be many risks with the usage of RwLock as I've proposed.

  • Tera is only "written" to during initial startup and a reload during zola serve
    • This means most of the time it's only read from, and since there's only one situation where data is written during runtime, deadlocks can't happen
  • The ShortcodeInvocationCounter encapsulates the usage of RwLock within the scope of its function method, making deadlocks in that case also impossible.

@Keats
Copy link
Collaborator

Keats commented Oct 23, 2024

@ARitz-Cracker i haven't forgotten about it, I just need to dedicate some time to that PR

@Keats
Copy link
Collaborator

Keats commented Nov 5, 2024

Ok I've spent some time on it finally, sorry for the delay.

I think my concern is with trying to make the markdown filter a 1:1 with the markdown rendering of pages/sections. The filter currently supports shortcodes but in hindsight I think it was probably a mistake (as well as something super niche AFAIK). If you want Zola-CommonMark rendered, put it in a page with render = false and grab the content from get_page. This way there is only one place that needs to handle shortcodes or Zola specific markdown things.
I would remove the changes to the filter from this PR entirely even if it's slightly inconsistent for now, and we can change that filter later. The changes should then be limited to mostly the pest parser and some tests + handling for nth.

As an aside, I'm expecting macros/components to look a lot like shortcodes in the future (see Keats/tera2#51) so maybe we will be able to just template the markdown content instead of having our own parser/shortcodes handling which would be very nice. If you have any feedback on that, please comment.

@ARitz-Cracker
Copy link
Contributor Author

ARitz-Cracker commented Nov 5, 2024

Hello @Keats , no worries for the delay, life happens, and I appreciate you taking a closer look.

Generally, I think the ideas in tera2 are very nice, and I'm very much looking forward to seeing progress on that front. I think it would greatly contribute to the "dehoustonification" (as tantacrul calls it) of Zola

As I've alluded to earlier, I also acknowledge that this PR contains some hacks to get around tera's current limitations and is far from the ideal solution when it comes to defining nested components written in markdown.

That said I imagine that:

  1. Tera2 is going still requires further work and refinement before it's production-ready
  2. Tera2 integration is going to be a backwards-incompatible change (which I know is an understatement) and when the time comes, it presents the opportunity to re-examine the architecture of Zola as a whole, I'm aware there was some talks of simplifying the pages vs sections workflow as well.

My motivation for this PR is to have a workable solution for being able to inline-define things like columns, grids, basically any situation that results in nested div's in multiple styles. I don't believe that misaligns with your vision of Zola as a whole, only that the underlying template system should have enabled the means to provide that functionality itself from the get-go, and I agree, but I'd like you to still consider merging the changes to the filter, as that can provide a comparable feature right now and in the meantime until the underlying requirements are complete for the more ideal solution.

@Keats
Copy link
Collaborator

Keats commented Nov 5, 2024

Nested shortcodes are completely fine, but why do you need it from the filter? What's the usecase there? I'd prefer to avoid changing code to add a feature that will be removed because people will start relying on it.

@ARitz-Cracker
Copy link
Contributor Author

To be honest, I pretty much just picked up from where from where #1475 left off and building upon the work of #1358. Though, now that you're saying that merge was a mistake, am I correct to assume that you would prefer having Shortcode::render expand shortcodes in the body before passing it to a filter? Because if so, then I'd be happy to explore that option and can submit a new PR with that in mind. Can't promise it'll be done this week, I'm pretty busy, but next week is possible.

@Keats
Copy link
Collaborator

Keats commented Nov 6, 2024

I think #1358 was the mistake.

Ideally we would have nested shortcodes as you have them right now in this PR for pages/sections and do not do any changes to the filter.

@alpaylan
Copy link

Is there a timeline for this merge? I'm working on my own website, and I will wait until this is merged if the time horizon is short enough(perhaps before 2025?). I can also try to contribute if there's any part that needs some work.

@ARitz-Cracker
Copy link
Contributor Author

Is there a timeline for this merge? I'm working on my own website, and I will wait until this is merged if the time horizon is short enough(perhaps before 2025?). I can also try to contribute if there's any part that needs some work.

I haven't had time to incorporate the feedback given due to a mix of work and personal obligations. This feature is important to projects I'm working on as well, though not immediately so.

I may have time this Sunday Sunday the 1st, though I am more likely to have time on the weekend of the 7th. Then it's up to Keats to determine if the changes are satisfactory

@spectria-limina
Copy link

Does this work with non-body arguments too? I have a lot of inline shortcodes for styling that I can't currently nest.

Some text with {{shortcode(t="{{nested(t='multiple formattings applied')}}")}}

@Keats
Copy link
Collaborator

Keats commented Dec 6, 2024

I don't think that would work but I could be wrong!

@ARitz-Cracker ARitz-Cracker mentioned this pull request Dec 24, 2024
3 tasks
@ARitz-Cracker
Copy link
Contributor Author

Closing in favour of #2748

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.

5 participants