Override wrappers

[redhead]

I thought that sentence "Specifying this property will override the wrappers for the type for this field." in the description of wrapper property of fieldConfig will do as says when specifying this in my field:

wrapper: ['bootstrapHasError']

Meaning I want a field without bootstrapLabel, but I want to get error classes on the container. However, this wraps the default type wrappers into another bootstrapHasError, not overriding them but adding additional one.

If I specify null it overrides it in the way I get no wrappers at all. I just want to remove one of the default wrappers. Thus, the documentation isn't detailed enough.

How can I do that?

[kentcdodds]

Good question. I believe the docs are unclear. If the type you have specified has wrappers assigned to it, then the only way to get rid of them is to use null which will remove all wrappers (as you have pointed out). There is not currently a way in angular-formly to completely override wrappers. So you'll have to create another type that has the wrappers you want. You can use formlyConfig.getType to copy properties from an existing type to a new, similar type that you create.

I'm thinking that as a breaking change for the next version, I'll make it so that if wrapper are specified on the field, then the type's wrappers will not apply. That seems to me to make the most sense. (while I'm at it, I'll change the name from wrapper to wrappers because most of the time it is specified as an array anyway.

[kentcdodds]

Would love feedback from anyone on this breaking change for the next version:

1. Any problem changing the term wrapper to wrappers as it's specified as an array most of the time?
2. Any problem removing the default wrapper behavior? Basically, if you specify the name 'default' then it will wrap all types. This is a little bit too magical I think. And I believe it's undocumented.
3. Any problem changing behavior so that if you specify a wrappers array on a field then rather than joining with the wrappers from the type, it entirely overrides them?
4. I've recently thought that <formly-transclude></formly-transclude> is quite long. Any suggestions on whether it should be something else/shorter?
5. Anything else anyone would like to see change in wrappers?

[redhead]

No problem with any of those. Just 4) is only cosmetic and I think it's not really neccessary to rename that, formly- prefix is used in formly everywhere as convention and transclude is just what it is and everyone knows what it does.

[pdemilly]

Should wrappers be a map then and we could overwrite the key with a new value or a null to remove it. Then you could test if it's an array you keep the old behavior if it's a map you implement the new behavior. The keys of the map could be label and error for example

[kentcdodds]

Good comments @redhead. I'll keep the formly-transclude then

@pdemilly, I love the idea of it being an object with keys. The problem with that is how to define order. Once you do that you get into an issue with priority... We've talked about this on #368. What we could do though is give a wrapper an ID and you could optionally specify a wrapper as an object with a name and an id. Something like:

wrappers: ['foo', {name: 'bar', id: 'barWrapper'}]

Though I still think that this is less simple than just overriding the type's properties altogether. Thoughts?

[redhead]

What about just making it a function then, giving it wrapper array parameter and let the user do whatever he wants with the list - pushing new, remove unwanted, replace, reverse... 

[kentcdodds]

Oh, I like that a lot. And that could be non-breaking as well... Problem is that it's not quite as obvious, so I think I'll still have a breaking change to make it override wrappers when specified, but I think optionally giving it a function opens a lot of flexibility.

[redhead]

Agreed, it's not obvious, but good documentation is the core of every project anyway :)

[ckniffen]

Any problem removing the default wrapper behavior? Basically, if you specify the name 'default' then it will wrap all types. This is a little bit too magical I think. And I believe it's undocumented.

I would like that feature to stay. I find it very useful especially when every field defines its own special things it is an easy way to extend types defined in a third party templates library.

[Mig1st4ck]

I would like this feature implemented.

Override should be the most intended behavior.

Using an object instead of an array could be cool so we could set it like this:

wrappers: {
    'override': false, /* a special key could be used to allow not to override */
    'bootstrapLabel': 10, /* using an int would set the priority of each key map */
    'bootstrapError': false || 0,
    // 'bootstrapXPTO': 0 /* any non declared wrappers would be set to false, unless override was set to true */
    'dynamic': {
        priority: 15,
        template: '<div></div>' /* this could even allow a dynamic wrapper on the fly... */
    }
}

This would pretty much give all needed functionality on this subject.

[kentcdodds]

Definitely not going to get into priority. I would hate to see the day where I see: priority: Infinity and priority: Infinity + 1 :-(

We need this to be simpler, not more complex.

[redhead]

Also, it isn't really transparent having numbers and booleans just next to wrapper name. I think that having a function that enables you to do what ever you want with an array of wrapper names is better approach.

[SteveShaffer]

Throwing my hat in: I also like the function approach. I'd just note that, especially if you're keeping the string and array options, you'll now have three options. I think that's mega-cool, but also a little magical and you'd at least want to make sure that sort of expectation is semi-consistent across the library. Are there other places that accept functions that don't currently boil down to arrays (like functions that return objects or anything)? I can't think of any but I don't know the whole library. In general, I think that's a great open, backwards-compatible, extensible general purpose approach to these sorts of override-heavy APIs:

String: 'string' --> ['string']
Array: ['arr', 'ay']
Function: function (existingArray) { /* do stuff */ return newArray; }

[cbs-greg]

Ah, I'm not going crazy, good to see confirmation that it doesn't override at the moment, I've spent the last few hours trying to override by specifying this property on a field.

I really like the function option that would allow complete freedom to manipulate the wrapper stack in any way we like. I would love to see that extension.

One thing to consider, is that you can also specify wrappers on a form, and it would make sense to have consistent behaviour (override or not) when specifying wrappers on forms and fields. So I think that adding wrappers makes sense as the default action (just update the documentation to say so).

You could add an additional option to both field and form called "wrapperBehaviour" with possible values "override" and "concat" (concat being the default). It's nice and stops it being a breaking change as well.

I'll go with a custom type for now to get around my problem. Thanks, we love formly! :)