posthtml / posthtml-components

A PostHTML plugin for creating components with HTML-friendly syntax inspired by Laravel Blade. Slots, stack/push, props, custom tag and much more.
https://posthtml.github.io/posthtml-components/
MIT License
15 stars 1 forks source link

Prevent attributes used for passing props from being added to first node #24

Closed cossssmin closed 1 year ago

cossssmin commented 1 year ago

Component:

<!-- sample.html -->
<p>{{ text }}</p>
<content></content>

Use it:

<x-sample text="Lorem ipsum">
  ... dolor sit amet
</x-sample>

Result:

<p text="Lorem ipsum">Lorem ipsum</p>
Test

Expected:

<p>Lorem ipsum</p>
  ... dolor sit amet

The text="Lorem ipsum" attribute makes no sense there in this case.

I understand the current state is a mix between a) wanting to pass data through attributes and b) wanting to pass attributes to the component, which I find a little confusing - but that's because I'm used to the way attributes work in posthtml-modules.

However the issue remains, in reality there are times where you need to use an attribute to pass data down to the component but you don't want that attribute actually rendered anywhere.

Would you consider differentiating between attributes that should be rendered and those that shouldn't, in some way?

Maybe use a : in front of the attribute to indicate that it shouldn't be rendered?

Like, with the example above:

<x-sample class="text-base" :text="Lorem ipsum">
  ... dolor sit amet
</x-sample>

Result:

<p class="text-base">Lorem ipsum</p>
  ... dolor sit amet

So the class attribute would be rendered on the first node, but the :text attribute that starts with the : would not be rendered, it would only make the text prop available to the component. Thoughts?

thewebartisan7 commented 1 year ago

You have to define props inside the script for avoid to be added as attributes to the first element. Example:

<script props>
  module.export = {
    text: props.text || 'default text' 
  }
</script>

<!-- sample.html -->
<p>{{ text }}</p>
<content></content>

This is also how Laravel Blade works, but also in VueJS is the same, where all props not defined as props is added as attributes to the first node, see https://vuejs.org/guide/components/attrs.html#attribute-inheritance

So the class attribute would be rendered on the first node, but the :text attribute that starts with the : would not be rendered, it would only make the text prop available to the component. Thoughts?

This could be another solution, but that means you need to always use : each time you use the component, while you need only to define once inside the component the props.

So you as component creator have the responsibility and full control what will not be added as attributes; and you don't need to worry about developers who use your component.

What do you think? Make sense?

cossssmin commented 1 year ago

Thanks, makes sense.

but that means you need to always use : each time you use the component, while you need only to define once inside the component the props.

I'm trying to think of a way of closing (or reducing) the compatibility gap with posthtml-modules.

Since you can also do 'inline' prop checks in a component like so:

{{ typeof prop !== 'undefined' ? prop : 'default value' }}

... I was trying to come up with a way to prevent attributes from being added if you're checking props this way, without first 'registering' supported props in a script tag.

So coming back to my example, it would be easier for people to transition from posthtml-modules to this plugin if you could do something like:

<!-- sample.html -->
<p>{{ typeof text !== 'undefined' ? text : 'default text' }}</p>
<content></content>

<!-- use it -->
<x-sample class="text-base" :text="Lorem ipsum">
  ... dolor sit amet
</x-sample>

<!-- result -->
<p class="text-base">Lorem ipsum</p>
  ... dolor sit amet

Maybe there's a better way instead of prefixed attributes like :text="", but you get the point - something that would help with backwards compatibility with posthtml-modules, so we make it trivial for people to switch :)

thewebartisan7 commented 1 year ago

I got your point.

But also using :text="" that means it's required to update all components usage inside code, which could require update several files where the component it's used, instead of single file adding the <script>. In case we could make this optionable. However as explained before it will still require to update existing code that use the component/module.

... I was trying to come up with a way to prevent attributes from being added if you're checking props this way

The props/locals inside the component HTML code is processed by expression plugin before the attributes, so the only way to archive this would means to parse the component HTML code looking for prop name, and save them to be used in process-attributes.js... Yes it could works, but actually would be hard to create a regex that found all locals/props since inside {{ ... }} could be any js expression.. I have to think a bit more about this. In case you have an idea, let me know.

thewebartisan7 commented 1 year ago

@cossssmin what if we create an array of all possible HTML attributes, https://www.w3schools.com/tags/ref_attributes.asp plus add an option to add more custom attributes, also using regex for example all attributes starting with data-, which will be treat as attributes and not as props? We could also include all attribute kebab case since prop for now can't be kebab case. I think this could solve this issue.

cossssmin commented 1 year ago

Yes, I actually saw that somewhere recently, they had a similar issue and were safelisting reserved HTML attributes to differentiate between them... can't remember where though :(

thewebartisan7 commented 1 year ago

Yes surely someone has similar requirements, will do a research about, maybe something already exist that we could use.

I don't see any potential issue using this solution, even if a new attribute is introduced, it can be added via option.

thewebartisan7 commented 1 year ago

@cossssmin I pushed the first draft for this feature.

You could test setting in package.json:

"posthtml-components": "github:thewebartisan7/posthtml-components#main"

Since I didn't find anything that we could use, I made this script for retrieve all valid, not depreacted HTML attributes: https://jsfiddle.net/1r8czt2h/

The script retrieve from this pages all valid HTML attributes https://www.w3.org/TR/html4/index/attributes.html and only for not deprecated HTML tags according to this list https://www.w3.org/TR/html4/index/elements.html

In the end it create an object with key as element name and values all supported element's attribute, see https://github.com/thewebartisan7/posthtml-components/blob/main/src/valid-attributes.js

Like before, if you define a prop inside the script, it will NOT be added to attributes, this for allow to manipulate the values of attributes. In this case is developer who add the prop/attribute to the element.

I will make some more tests in next days and then we can release new tag. Also I still need to add options to add new custom attributes, and set a blacklist so that it's possible to remove the default attributes.

It should not add any breaking changes, as tests still pass, but need to test also with Bootstrap UI and mine project to be sure.

I will update you.

thewebartisan7 commented 1 year ago

@cossssmin any feedback about?

I was not yet able to finish this, and not sure will have chance before incoming holiday.

But please share with me if you have any feedback so far.

Thanks!

cossssmin commented 1 year ago

Hey @thewebartisan7 sorry this took so long for me to get back to, I just tested now and it looks good. And yes, the options for additional/blacklisted attributes would be useful đź‘Ť

thewebartisan7 commented 1 year ago

Hi @cossssmin, nice to hear from you and Happy new year! No problem, I was also a bit busy with holiday and work. I will check into this during this week and release new version.

Are you still planning to update docs? Did you update something?

Because I have to change docs a bit after this release, so if I change something now, just to be align with any changes you have made.

cossssmin commented 1 year ago

Happy new year!

No I haven't yet updated any docs, so don't worry go ahead and release the new version and we take it from there.

Besides this, do you think there's anything else that needs to be done or is it ready for v1.0?

thewebartisan7 commented 1 year ago

Apart this, there is async plugin support (see https://github.com/thewebartisan7/posthtml-components/issues/21)

But I am not yet sure about, it's all depend on how much changes this require.

We could also release stable release without it.

cossssmin commented 1 year ago

Yeah, that can come later in a feature release đź‘Ť

thewebartisan7 commented 1 year ago

@cossssmin finally I was able to complete this and I have release the first RC tag, see https://github.com/thewebartisan7/posthtml-components/releases/tag/v1.0.0-RC.1

The main problem was that I release that the page from where I get the list of html attributes are not considering HTML5 tags and attributes, this one https://www.w3.org/TR/html4/index/attributes.html

So it take me time to find a list and there is not a complete list like this, so I have to combine from this pages:

https://html.spec.whatwg.org/multipage/indices.html https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes

Which is not even fully complete as I didn't found for example aria attributes, so I added this in safelist options.

I have also updated the docs accordingly, but very quickly, and most probably will need to review it again.

I try to make fully customizable the list of valid-attributes so you can add attributes to specific tag, or override all attributes for specific tag. You can also safelist or blacklist attributes to all tags. In the docs in Attributes section there is an example.

Looking for your final feedback and then we can release the stable one.

About any query of RC 1 we can use this discussion https://github.com/thewebartisan7/posthtml-components/discussions/27

Thanks!

cossssmin commented 1 year ago

Sweet, getting on testing it right now 🤩

cossssmin commented 1 year ago

Testing this today and I'm getting an error when testing out namespaces, am I doing something wrong?

I have this config:

{
  root: './',
  folders: [ 'src/components', 'src/layouts', 'src/templates' ],
  namespaces: [
    { 
      name: 'theme-dark', 
      root: './src/theme-dark', 
    },
    {
      name: 'theme-light',
      root: './src/theme-light',
    }
  ]
}

And this directory structure:

src/
├─components/
├─layouts/
├─templates/
├─theme-dark/button.html
└─theme-light/button.html

Using them like this:

<x-theme-dark::button></x-theme-dark::button>
<br>
<x-theme-light::button></x-theme-light::button>

... fails with the following error:

[components] For the tag x-theme-dark::button was not found any template in the defined namespace's base path [my project path]\src\theme-dark.

Moving the button.html files inside a components folder, so it's found at theme-dark/components/button.html doesn't work either, same error.

thewebartisan7 commented 1 year ago

It's strange that tests for namespace passed successfully. I will try to reproduce your issue and let you know. Your setup seem fine to me.

thewebartisan7 commented 1 year ago

@cossssmin I got now where is the problem in your folders structure.

Your folder structure inside each namespace must folllow the same structure of your root folders.

So in your case should be:

src/ ├─components/ ├─layouts/ ├─templates/ ├─theme-dark/components/button.html └─theme-light/components/button.html ├─theme-dark/layouts └─theme-light/layouts ...

Here is the search function which you can see how it works:

https://github.com/thewebartisan7/posthtml-components/blob/main/src/find-path.js#L102-L119

This search function is being used for both root and namespace root, and take into account always the options.folders.

Why this?

Because imagine you have the same filename for components and layouts, or any other folders.

So basically a namespaces follow the same structure you have in the main root.

Let me know if I explain well myself and if you have any concerns about.

Thanks

cossssmin commented 1 year ago

Ah, ok, so I actually need to have them like this:

src/
├─components/
├─layouts/
├─templates/
├─theme-dark/src/components/button.html
└─theme-light/src/components/button.html

Works now, thanks for clearing that up.

I'll go ahead and close this, original issue has been resolved too đź‘Ť

thewebartisan7 commented 1 year ago

@cossssmin you can also keep shared root src of folders in root

{ root: './src', folders: [ 'components', 'layouts', 'templates' ], namespaces: [ { name: 'theme-dark', root: './src/theme-dark', }, { name: 'theme-light', root: './src/theme-light', } ] }

Then you can:

src/
├─components/
├─layouts/
├─templates/
├─theme-dark/components/button.html
└─theme-light/components/button.html
├─theme-dark/layouts
└─theme-light/layouts

Because path is always main root + folders, or namespace root + folders