kgiszewski / Archetype

Archetype is an Umbraco 7 property editor that wraps other installed property editors.
https://github.com/kgiszewski/ArchetypeManual
MIT License
89 stars 54 forks source link

Custom label template broken #429

Closed kipusoep closed 6 years ago

kipusoep commented 7 years ago

With version 1.16.0 my custom UrlPicker label template broke. The labels are empty but appear when I expand an item. It works with v1.15.1.

The code:

var UrlPickerTemplate = {};

UrlPickerTemplate.getTitle = function (value, scope) {
    //this is the property model
    if (value.length) {
        var firstValue = value[0];
        return ArchetypeSampleLabelTemplates.UrlPicker(firstValue, scope, { propertyName: "name" });
    }

    //if you wanted to get the name of the content instead, you'd have to get it from the server here since it's not in the model

    return "";
};
kgiszewski commented 7 years ago

@kipusoep I'll look into this today. Thx for the report.

kgiszewski commented 7 years ago

Curious if this is related to #428 @Nicholas-Westby

kipusoep commented 7 years ago

Looking forward to your findings! :-)

Nicholas-Westby commented 7 years ago

Somewhat related. I think it's because ArchetypeSampleLabelTemplates.UrlPicker calls archetypeCacheService.getEntityById which initially returns null. Subsequent calls will eventually return a value after the cache has been populated.

I could modify archetypeCacheService.getEntityById to return a promise, but that won't help people that are expecting the entity to be directly returned. Maybe I could initiate an additional digest cycle to force it to re-run after the asynchronous call returns, but that could lead to performance issues and unintended side-effects.

In short, it would be relatively easy to fix this particular issue, but fixing all issues others may see will be more challenging, so I'll have to think on that a bit (also open to other ideas).

Nicholas-Westby commented 7 years ago

My current plan is to take both routes. That is, modify archetypeCacheService.getEntityById so it will return a promise if you pass an optional boolean parameter to force it to do so.

Also, I can initiate an additional digest cycle. To reduce the number of digest cycles, I can debounce it so it runs no more frequently than once every half second.

The first method (i.e., the promise) would be preferred, but the second method (i.e., digest cycle) should at least get things working (to some degree) for others that are already expecting the non-promise result.

Nicholas-Westby commented 7 years ago

FYI, I don't expect to be able to get to this until next week. If you like, I can probably squeeze in a quick fix for the promise variation tonight, which would require changes to UrlPickerTemplate.getTitle. It'd basically have to change to something like this:

UrlPickerTemplate.getTitle = function (value, scope) {
    //this is the property model
    if (value.length) {
        var firstValue = value[0];
        return ArchetypeSampleLabelTemplates.UrlPicker(firstValue, scope, {
            propertyName: "name",
            returnPromise: true
        });
    }

    //if you wanted to get the name of the content instead, you'd have to get it from the server here since it's not in the model

    return "";
};

What do you think, @kgiszewski and @kipusoep?

kgiszewski commented 7 years ago

i think i might have a fix that is backwards compatible. I'll post soon.

kgiszewski commented 7 years ago

I'm gonna grab some lunch: https://github.com/kgiszewski/Archetype/pull/430/files?w=1

Not sure if this is a good or bad idea :) But essentially I'm raising an event that says to clear out any 'loaded' label templates if a new entity has been added to the cache. I have to test this at scale (many fieldsets) and with promises. But so far so good.

Nicholas-Westby commented 7 years ago

Don't have time for a thorough review at the moment, so I'm not entirely sure what's going on. My main concern is that I don't see that you are tying the specific entity being updated to the fieldset title that requires that entity. So I wonder if you would be prematurely considering fieldset titles to be loaded.

Then again, I didn't read it too thoroughly, so I could well be missing something.

kgiszewski commented 7 years ago

I stumbled upon an issue in the caching with datatypes, so i resolved that and i'm still going down the rabbit hole. Don't sweat a fix right away.

Nicholas-Westby commented 7 years ago

Cool. FYI, I was tinkering with the way things are cached too for that other issue: https://github.com/Nicholas-Westby/Archetype/commit/65fd9e0ab4f86a5fa33082a3963536be60de4070

kgiszewski commented 6 years ago

Ok @kipusoep the fix i pushed and released today should take care of some of the bugs we saw. However for your exact issue; you'll wanna try to use the built-in label for URL picker and your template would look something like this:

{{url}} where url is the name of the URL picker.

I'm closing this for now; please ping back if you have any troubles (#431).