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

[5.x] Antlers: Adds support for disabling Antlers entirely inside tag parameter content #8887

Merged
merged 6 commits into from
Feb 29, 2024
Merged

[5.x] Antlers: Adds support for disabling Antlers entirely inside tag parameter content #8887

merged 6 commits into from
Feb 29, 2024

Conversation

JohnathonKoster
Copy link
Contributor

@JohnathonKoster JohnathonKoster commented Oct 28, 2023

This PR adds support for disabling the parsing of Antlers entirely when supplying parameter values by introducing new syntax.

For example, let us consider the following form tag using the existing syntax to escape { and } safely inside parameter content:

{{ form:contact x-data="@{ open: false, toggle() @{ this.open = ! this.open @} @}" }}
    ...
{{ /form:contact }}

With the changes in this PR, we could now just prefix our parameter name with a single backslash (\) character:

{{ form:contact @x-data="{ open: false, toggle() { this.open = ! this.open } }" }}
    ...
{{ /form:contact }}

Notes/considerations:

  • There is no way to get back into Antlers inside the parameter when using the \ syntax. This syntax completely disables the recursive parser
  • Breaking change concerns: none. The \parameter="something" is currently illegal syntax, and won't produce parameters at all
  • Why not use @parameter="something" to indicate the escaped content? While this would be nice for consistency, it would be a breaking change since using parameters like @click="doTheRoar()" is already supported

@ryanmitchell
Copy link
Contributor

Just thinking out loud, but if you needed the output of backend variables in the JS, would something like this work?

{{? $js = '{ open: '.$variable.', toggle() { this.open = ! this.open } }' ?}}
{{ form:contact :\x-data="js" }}
    ...
{{ /form:contact }}

@JohnathonKoster
Copy link
Contributor Author

JohnathonKoster commented Oct 29, 2023

Just thinking out loud, but if you needed the output of backend variables in the JS, would something like this work?

{{? $js = '{ open: '.$variable.', toggle() { this.open = ! this.open } }' ?}}
{{ form:contact :\x-data="js" }}
    ...
{{ /form:contact }}

For that, you could accomplish that without this PR and just do :x-data="js".

I could likely get something similar to the following working without a ton of additional effort, but would need to gauge utility/interest (potentially feedback from Core on whether to continue with it):

{{ form:contact \x-data="{ open: ${open | bool_string}, toggle() { this.open = ! this.open } }" }}
    ...
{{ /form:contact }}

Would introduce some new syntax rules when using \ escaped parameters:

  • To emit a $ literal that is followed by a {, we would need to escape it \${
  • A $ not followed by a { would not need to be escaped
  • Once inside the first ${ ... } region, we would not need to continue prefixing { with $ as we are "back" in Antlers-land

There would be a number of things to consider when adding support for this:

  • Documentation around how to escape } once back inside Antlers land (already possible, but not a well-known thing)
  • Ensure consistent behavior if a tag is utilized inside the nested Antlers (really awful example, but wouldn't be surprised to see something like this after some time):
{{ some:thing \escaped="This {is} {all} escaped ${another:tag \param="nested ${fun:stuff \here="{we} ${go}"}"}" }}

Not directly related to the functionality of the runtime itself:

  • Syntax highlighting support
  • Editor support
  • Formatter support
  • Supportive error messaging, documentation, etc.

@jasonvarga
Copy link
Member

Breaking change concerns: none. The \parameter="something" is currently illegal syntax, and won't produce parameters at all
Why not use @parameter="something" to indicate the escaped content? While this would be nice for consistency, it would be a breaking change since using parameters like @click="doTheRoar()" is already supported

Adding another symbol is a bit confusing IMO. "Use a @ to escape these, but a \ to escape these..."

I would personally prefer @ to be used as the character to be consistent.

That would make it a breaking change, so we could just have this target 5.x.

Along with making attributes on form-rendering tags (like form:create, user:password_form, etc) require an attr: prefix, like we do on the vite tag. The attr: prefix already works on form tags, but also non-prefixed params. In v5 we can enforce the prefix.

So, as part of the upgrade guide, you'd need to do this on your form tags:

{{ form:create
     in="contact"
     redirect="/thanks"
-    class="foo"
-    @submit="submitForm"
+    attr:class="foo"
+    attr:@submit="submitForm"
}}

As for this:

I could likely get something similar to the following working without a ton of additional effort, but would need to gauge utility/interest (potentially feedback from Core on whether to continue with it)

Let's just stick with escaping the whole parameter content for now. This sounds super complicated. 😄

@jackmcdade
Copy link
Member

I concur with @jasonvarga.

Copy link
Member

@jasonvarga jasonvarga left a comment

Choose a reason for hiding this comment

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

Please target master, and just change the prefix to @.

We can tackle the form attr: prefix changes in a separate PR.

@JohnathonKoster
Copy link
Contributor Author

JohnathonKoster commented Oct 30, 2023

Done. Changed the PR target to master and applied the requested changes.

Let me know if you need anything else on this.

@JohnathonKoster JohnathonKoster marked this pull request as draft October 30, 2023 22:06
@JohnathonKoster JohnathonKoster changed the base branch from 4.x to master October 30, 2023 22:06
@JohnathonKoster JohnathonKoster changed the title [4.x] Antlers: Adds support for disabling Antlers entirely inside tag parameter content [5.x] Antlers: Adds support for disabling Antlers entirely inside tag parameter content Oct 30, 2023
This adjusts a previous test to account for this change
@JohnathonKoster JohnathonKoster marked this pull request as ready for review October 30, 2023 22:52
@jasonvarga
Copy link
Member

I just updated master to be in sync with 4.x. Can you merge master into your branch? Then there won't be 200+ changed files. 😆

@JohnathonKoster
Copy link
Contributor Author

I just updated master to be in sync with 4.x. Can you merge master into your branch? Then there won't be 200+ changed files. 😆

Done!

# Conflicts:
#	tests/Antlers/Parser/NodeParametersTest.php
Copy link
Member

@jasonvarga jasonvarga left a comment

Choose a reason for hiding this comment

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

Hey John, this is great.

But after merging master back into this, there's a failing test. I'm not sure where it was introduced. I don't think it's this PR though, but maybe you can figure it out?

There's also one deprecation warning - don't worry about that for now.

@JohnathonKoster
Copy link
Contributor Author

JohnathonKoster commented Feb 29, 2024

Hey John, this is great.

But after merging master back into this, there's a failing test. I'm not sure where it was introduced. I don't think it's this PR though, but maybe you can figure it out?

There's also one deprecation warning - don't worry about that for now.

Looks like the space was trimmed out from the template string during the merge. Digging through different branches, I can't find any that had removed this space (and is present on master). Possibly related to merging in the changes for shorthand variable syntax? 🤔

image

@jasonvarga
Copy link
Member

You're right, it was in that merge. My fault. Must have been my editor automatically stripping trailing spaces when I saved for that merge conflict, and I didn't notice. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants