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

Feature Request: Custom Attribute Parsing #1691

Open
ndesmic opened this issue Jun 27, 2019 · 12 comments
Open

Feature Request: Custom Attribute Parsing #1691

ndesmic opened this issue Jun 27, 2019 · 12 comments
Labels
Feature: Want this? Upvote it! This PR or Issue may be a great consideration for a future idea.

Comments

@ndesmic
Copy link

ndesmic commented Jun 27, 2019

Stencil version:

I'm submitting a:

[ ] bug report
[x ] feature request
[ ] support request => Please do not submit support requests here, use one of these channels: https://stencil-worldwide.herokuapp.com/ or https://forum.ionicframework.com/

Current behavior:
Stencil only seems to provide string and boolean values via attributes.

The current work around seems to be setting an intermediate property and using watch to set another property which is not ideal. https://medium.com/@gilfink/using-complex-objects-arrays-as-props-in-stencil-components-f2d54b093e85

Expected behavior:
A prop should have a custom conversion function that determines how attributes convert into property value, and also how properties reflect into attributes.

Steps to reproduce:
Example: I would like a component that takes an ISO Date String as an attribute but the property should be the actual date object. Maybe something similar to:

@Prop({
    attrSerialize: date => date.toISOString(),
    attrDeserialize: str => new Date(str),
    reflect: true
}) 
Date dateValue

Example: I would like to use a JSON string and convert it into a complex object.

@Prop({
    attrSerialize: obj => JSON.stringify(obj),
    attrDeserialize: str => JSON.parse(str),
    reflect: true
}) 
complexObject

Example: I would like to take a comma delimited list and convert it into an array.

@Prop({
    attrSerialize: arr => arr.join(","),
    attrDeserialize: str => str.split(",").map(x => x.trim()),
    reflect: true
}) 
arrayOfString

Other information:
Related issue: #1155

The related issue raises performance concerns with built-in object conversion but is that still a problem if the conversion is user provided?

If not considered, is the current work around the best-case for this pattern? Are there other alternatives?

@ionitron-bot ionitron-bot bot added the triage label Jun 27, 2019
@ndesmic ndesmic changed the title Feature Request Custom Prop Parsing Feature Request: Custom Attribute Parsing Jun 27, 2019
@thomasnaudin
Copy link

I couldn't agree more, especially for JSON attributes.
For my use case, I'm trying to embed Leaflet map in a Stencil component.
Be able to pass Leaflet config (points, polygons, etc) via some data attribute would be a real betterment.

@mboudreau
Copy link

Definitely agree with this. In the meantime, I've adopted this not overly ideal pattern:

@Prop({attribute: 'data'}) dataAttr: string;
@Prop({reflect: false}) data: any[];
@Watch('dataAttr')
onDataChange(value: string) {
    this.data = JSON.parse(value);
}

What I would like to see from the Stencil team for parsing complex types would be something like this:

@Prop({serializer: Serializer.JSON}) data: any[];

Where Serializer.JSON is an internal serializer provided by stencil (there could be one for CSVs for instance or any other number of internal or custom ones) where it would have to implement this:

interface StencilSerializer<T> {
    serialize: (value:T) => string;
    deserialize: (value:string) => T;
}

This would allow flexibility across the board for a whole range of options. However, looking at the stencil code, the @Prop decorator doesn't seem to act as a regular decorator and seems to be parsed as part of the compilation step. I'm kinda surprised that Ionic hasn't had the need for more complex types in attributes but I can appreciate the complexity of it all. At the very very least,, it should be possible to just have an option that flags the property as being a JSON object and is serializable/deserialization, something like @Prop({json:true}). This would solve many of my issues and reduce the need for all this boilerplate for each complex property.

Happy to work on the PR if necessary to get this in.

@claviska
Copy link
Contributor

This comes up a lot, and although it's an unpopular opinion, I'll reiterate what I said here:

Even if Stencil formed the opinion that it should be one or the other, it now has to parse and decode the value when getting and setting. This will cause performance problems with large object and arrays, not to mention a nasty DOM if you were to reflect them.

Even if Stencil allowed it, I'd consider it an antipattern. I know it seems nice to set simple arrays and objects in your HTML, but if you really want to do that you'll need to handle it in your component. In my experience, it's just not worth the boilerplate and performance hit you're going to undertake.

Essentially, this feature will encourage a bad practice that results in slow components, particularly those handling large data sets and frequently changing attributes.

@ndesmic
Copy link
Author

ndesmic commented Jan 25, 2021

This comes up a lot, and although it's an unpopular opinion, I'll reiterate what I said here:

Even if Stencil formed the opinion that it should be one or the other, it now has to parse and decode the value when getting and setting. This will cause performance problems with large object and arrays, not to mention a nasty DOM if you were to reflect them.
Even if Stencil allowed it, I'd consider it an antipattern. I know it seems nice to set simple arrays and objects in your HTML, but if you really want to do that you'll need to handle it in your component. In my experience, it's just not worth the boilerplate and performance hit you're going to undertake.

Essentially, this feature will encourage a bad practice that results in slow components, particularly those handling large data sets and frequently changing attributes.

Does it? I'm not familiar with the internals of stencil but what I'd expect is that this happens on the attributeChangedCallback level. Properties are always of the concrete type and could have simple setters, only changing the attribute itself would cause parsing. It would be an anti-pattern to pass dynamic data through an attribute versus a property but this is primarily for static usage outside of the stencil VDOM.

@rubenprins
Copy link

To add my 2 cents, it would be really convenient if you could set attribute converters/serializers if you want to be able to specify almost-primitive values like dates, such as

<my-calendar selected="2021-05-05"></my-calendar>

Currently, @Prop() current: Date simply does not work, so you need two properties (the 'proper' typed property and a shadowed string-value property), both mutable, both publicly visible, both linked to an attribute (a Prop can't be just a property), a watch to keep the two in sync, and a life cycle method to sync the initial prop values. Just to be able to specify a date in plain HTML.

Note that in HTML it's actually not uncommon to have more complex attribute syntax than just string or number. Just think of style and datetime for <time>, <del>, and <ins>.

In case of value for <input type="date|datetime|...">, the value property is a DOMString and the parsed value is surfaced as valueAsDate, but that's actually for back-compat. And there's no value-as-date attribute.

@johnjenkins
Copy link
Contributor

johnjenkins commented Jun 24, 2021

are @Prop getters / setters a good compromise here?
#2899

@rubenprins
Copy link

@johnjenkins In my case, that could work. At least it would be a lot better than the whole mess I described above, including checks for not updating unchanged values. (And it's better than 'unknown class node' 😄).

One thing I don't know how to handle properly would be uglies like Date objects (or any mutable object, really). For Date properties, the getter should actually always return a copy, since returning a mutable object like that would interfere with change detection (and internal state). But would returning copies from the getter not trip up the change detector? <input>'s valueAsDate actually functions like that.

@splitinfinities
Copy link
Contributor

This is an interesting idea! I'm going to label this for us to review later.

@splitinfinities splitinfinities added Feature: Enhancement Feature: Want this? Upvote it! This PR or Issue may be a great consideration for a future idea. and removed triage labels Aug 3, 2021
@ionitron-bot ionitron-bot bot added the ionitron: stale issue This issue has not seen any activity for a long period of time label Sep 2, 2021
@ionitron-bot
Copy link

ionitron-bot bot commented Sep 2, 2021

Thanks for the issue! This issue is being closed due to inactivity. If this is still an issue with the latest version of Stencil, please create a new issue and ensure the template is fully filled out.

Thank you for using Stencil!

@ionitron-bot ionitron-bot bot closed this as completed Sep 2, 2021
@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 2, 2021
@rwaskiewicz
Copy link
Member

Hey ionitron-bot, we didn't mean to close this. Bad bot!

@rwaskiewicz rwaskiewicz reopened this Sep 3, 2021
@rwaskiewicz rwaskiewicz removed the ionitron: stale issue This issue has not seen any activity for a long period of time label Sep 3, 2021
@ionic-team ionic-team unlocked this conversation May 30, 2023
@rwaskiewicz
Copy link
Member

Relates to: #2533

@maxpatiiuk
Copy link

Resolving #1359 would remove the need for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Want this? Upvote it! This PR or Issue may be a great consideration for a future idea.
Projects
None yet
Development

No branches or pull requests

9 participants