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

Fusion Attributes with dots don't get correct rendered #3112

Closed
jonnitto opened this issue Sep 16, 2020 · 19 comments · Fixed by #3442
Closed

Fusion Attributes with dots don't get correct rendered #3112

jonnitto opened this issue Sep 16, 2020 · 19 comments · Fixed by #3442

Comments

@jonnitto
Copy link
Member

If I want to use special attributes which contains dots, these attributes don't get rendered correct

Input Output
x-on:click.away x-on:click
x-on:keydown.window.escape Array to string conversion exception

You may ask why to use such attributes. Well, if you use libraries like Alpine.js you'll need these kind of attributes.

Currently, I solved this like this: x-on:keydown_DOT_window_DOT_escape and add a @process like this ${String.replace(value, '_DOT_', '.')}. But I don't really like this aproach…

@jonnitto jonnitto changed the title Fusion Attributes with dots don't get correct Fusion Attributes with dots don't get correct rendered Sep 17, 2020
@cvette
Copy link
Contributor

cvette commented Sep 23, 2020

That's strange. I would've expected that writing it like this 'x-on:keydown.window.escape' = 'test' will work. Without the ' it's interpreted as a nested object path, that's correct.

@kitsunet
Copy link
Member

The fusion parser doesn't understand quoted keys, it's not something it understands. That might be possible to implement but I am not sure it's easy or even a great idea. Looking at Alpine.js website I would say they do not conform to HTML spec for attribute names (dots are allowed but colons and a couple of other things I don't think).

@cvette
Copy link
Contributor

cvette commented Sep 23, 2020

The fusion parser doesn't understand quoted keys, it's not something it understands. That might be possible to implement but I am not sure it's easy or even a great idea. Looking at Alpine.js website I would say they do not conform to HTML spec for attribute names (dots are allowed but colons and a couple of other things I don't think).

But it seems to be supported or at least it has been supported at some point. But I haven't tested this any further. This is from the Fusion parser:

 @?[a-zA-Z0-9:_\-]+              # Unquoted key
 |"(?:\\\"|[^"])+"               # Double quoted key, supporting more characters like underscore and at sign
 |\'(?:\\\\\'|[^\'])+\'          # Single quoted key, supporting more characters like underscore and at sign`

@radmiraal
Copy link
Contributor

I'm also running into the issue, and reading it I'm wondering 2 things:

  • IMHO fusion finds stuff it doesn't understand and then strips it away. Why? That seems too greedy to me.
  • Why would fusion enforce html specs? Isn't it up to the integrator what the output would be? HTML is just one possible form right?

@Sebobo
Copy link
Member

Sebobo commented Jan 27, 2021

Quoted keys basically work. I use something like this in the SEO package:

'@context' = ${props.context}

But dots are still a no-go.

@albe
Copy link
Member

albe commented Jan 27, 2021

That's a valid point IMO. No matter if the value doesn't conform to standard X/Y/Z, there needs to be a way to escape a sequence of characters in the language syntax, such that the output is unaltered.

What is the reason that a dot within a quoted property needs to be interpreted (and removed)?

@radmiraal
Copy link
Contributor

What I still fail to understand is why this wouldn't work. Is afx passing the attribute to fusion to do magic?

Like:

afx`
    <tag my.fusion.object="foo"/>
`

should turn into generatedattributename="foo" ?

And even for:

afx`
    <tag @click="foo" />
`

Fusion has no notion of @click, so why would it be stripped from the parsed output?

I didn't dig code yet, so I'm not sure how afx / fusion work in detail. But I can imagine afx not passing attributes that start with an '@' if they are not known to fusion anyway. And for the dotted notation I personally can not think of a use case that would really require parsing Fusion objects in the attribute name...

@radmiraal
Copy link
Contributor

I would love to know how the "dotted annotation" should work by the way... If I can really use a fusion object in the attribute name something like this should work too ;) but then I'd love to know what the syntax should be like :p Currently it outputs <button props="console.log('bar')">click</button>

foo = Neos.Fusion:Component {
    clickaway = '@on.click.away'
    renderer = afx`
        <button props.clickaway="console.log('bar')">click</button>
    `
}

@Sebobo
Copy link
Member

Sebobo commented Jan 27, 2021

In your example button is converted to a ´Neos.Fusion:Tag´ with tagName = button and AFX turns all first level props into children of the attributes inside Neos.Fusion:Tag if it's a HTML tag.

For Fusion components in AFX all attributes are directly passed as props.
And with the dotted notation you can override the deeper implementation.

See also https://github.com/neos/fusion-afx

@radmiraal
Copy link
Contributor

radmiraal commented Jan 27, 2021

ok, so to make it "work" my example should create the following (pseudo code :p ):

foo = Neos.Fusion:Tag {
    tagName = 'button'
    attributes {
        '@on.click.away' = "console.log('bar')"
    }
    content = 'click'
}

where the problem with the dotted annotation would be "we don't know if the deeper path with .away" exists?

sidenote:
Afaik the dots in the html attributes are actually valid... So by using the fusion syntax in the afx template part of the HTML spec in which the afx template is used cannot be used...

@mficzel
Copy link
Member

mficzel commented Jan 27, 2021

So this would require two changes to the afx service.

  1. Limit the exception for meta attributes to @if and @process
  2. In HTML Tags put quotes around attributes with '@' or dots

@radmiraal
Copy link
Contributor

  1. Limit the exception for meta attributes to @if and @process

That seems to be a viable solution for this part of the issue: neos/fusion-afx#34

  1. In HTML Tags put quotes around attributes with '@' or dots

Can we make up a list of examples with dots in the attribute name that would actually lead to a valid / expected result in afx which do not start with @if or @process ?

@mficzel
Copy link
Member

mficzel commented Jan 27, 2021

Structured apis are not uncommon in fusion;

A very basic example is tag-attributes. Offcourse this can be written in afx easier but it is valid:
<Neos.Fusion:Tag attributes.style="" />

Also NeosFusionForm uses things like this:
<Neos.Fusion.Form:Form form.target.action="sendOrder" form.target.controller="Order" >

@radmiraal
Copy link
Contributor

radmiraal commented Jan 27, 2021

Ok, so

<tag x-on:foo.bar.baz="" />

Would conflict if one has code like

prototype(Neos.Fusion:Tag) {
    x-on:foo {
        bar {
            baz = 'something'
        }
    }
}

And there's basically no fair chance to detect that probably.

Can't we simply configure a list of "ignored" prefixes? Like:

Neos:
    Fusion:
        ignoreAttributePatterns:
            - "^x-on:.*"

While playing around with the : I noticed the parsing of those also lead to an exception (just like x-on:keydown.window.escape above)

foo = Neos.Fusion:Join {
    title:foo = "asd"
}

is valid, but this leads to an error:

renderer = afx`
    {props.title:foo}
`

@mficzel
Copy link
Member

mficzel commented Jan 27, 2021

The main switch we do in afx is between html tags and fusion-component tags. The latter ones are defined as containing an ':'. I think as long as we limit the effects to html tags it would be fine and fair to support dots in attributes

so: <tag x-on:foo.bar.baz="" /> could be quoted in fusion but not <Vendor:Site x-on:foo.bar.baz="" />

Question is is this a rule that people will understand?

@Sebobo
Copy link
Member

Sebobo commented Jan 27, 2021

I'm not in favour of introducing too many behaviours for html tags that are different for components. Already the attributes behaviour sometimes confuses people and makes AFX less transparent (even though it helps a lot).

@jonnitto
Copy link
Member Author

What do you think about the solution to something like this: <tag 'x-on:foo.bar.baz'='bar' /> If the attribute is quoted, use it as a single key. So
<tag 'x-on:foo.bar.baz'='bar' />
would lead to

Neos.Fusion:Tag {
    tagName = 'tag'
    attributes {
        'x-on:foo.bar.baz'='bar' 
    }
}

@radmiraal
Copy link
Contributor

I've been thinking about it during a walk. I was on the track that having the dotted annotation being directly passed to the fusion component is wrong, but I changed my mind. I think a frontend developer would expect the dotted notation to translate to the object structure of the component. And that's exactly what afx / fusion is doing now.

So then the question would be what syntax would be most self-explaining / easiest to understand / handle. Patterns could work, an extra registered meta char (that marks 'skipping') could work, but maybe the quoting is the closest to actual Fusion and it's probably the most performant one.

And if we would also require the quotes for the @click.away use case it would also not be a problem to keep passing everything unquoted starting with a @ to Fusion, so it would tackle the @-problem and the dot problem I guess.

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue Sep 26, 2021
fixes neos#3112

- Fusion parser: dont split paths at dots, who are inside quotes.
- AFX: allow quoted attribute identifier.

Example:
AFX:
```html
<div '@foo.bar'='baz'></div>
```

Fusion:
```ts
Neos.Fusion:Tag {
    attributes.'@foo.bar' = 'baz'
}
```
Fusion ast (excerpt):
```php
'attributes' => [
    '@foo.bar' => 'baz'
]
```

Before this, AFX doesnt allow quoted attribute identifier,
and the Fusion Parser split at dots in quotes too, making those paths nested.
@mhsdesign
Copy link
Member

mhsdesign commented Sep 27, 2021

i played around with this and made a pr. With ~10 lines of changed code this would be solved (the rest are tests ^^) #3442

As in the pr explained, there a two problems:

  • AFX doesnt allow quoted attribute identifiers
  • The Fusion parser splits at dots inside quotes too, making those paths nested.

That means that allowing this AFX <div '@foo.bar'='baz'></div> alone wont really solve it since the Fusion parser doesnt understand the transpiled Fusion 'correctly' (at least it doesnt understand to 'protect' dots, it understands quoted paths already).

so this Fusion :

Neos.Fusion:Tag {
    attributes.'@foo.bar' = 'baz'
}

should be parsed to:
(which will be rendered as <div @foo.bar="baz">)

{
    "__objectType": "Neos.Fusion:Tag",
    "attributes": {
        "@foo.bar": "baz"
    }
}

but it gets parsed to:
(which will have the the effect, that only this will be rendered: <div @foo="baz">)

{
    "__objectType": "Neos.Fusion:Tag",
    "attributes": {
        "@foo": {
            "bar'": "baz"
        }
    }
}

@jonnitto jonnitto added this to Reviews Needed in Neos & Flow 7.3 LTS Release Board Oct 5, 2021
Neos & Flow 7.3 LTS Release Board automation moved this from Reviews Needed to Done Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
8 participants