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

Static Commands in MarkupControl #928

Merged
merged 18 commits into from
Feb 17, 2021

Conversation

Mylan719
Copy link
Contributor

I try to use static command as much as possible to make my pages faster. However I ren into a severe limitation. It is impossible to bind Command property to static command binding in the markup control.
Example:
Let's assume I have DeviceDetail markup control used in the page like so:

<cc:DeviceDetail Save={staticCommand: _root.Detail = service.Save(Detail); _alert.Show('Item saved')} />

I wanted to implement the DeviceDetail control as follows:

@viewModel MyProject.DeviceViewModel
@baseType MyProject.Device 
<div>
<!-- ... --/>
    <dot:Button Click={staticCommand: _control.Save()} Text="Save"/>
</div>

Code behind:

public class Device : DotvvmMarkupControl {
public Command Save
        {
            get { return (Command)GetValue(SaveProperty); }
            set { SetValue(SaveProperty, value); }
        }
        public static readonly DotvvmProperty SaveProperty
            = DotvvmProperty.Register<Command, Device>(c => c.Save, null);
}

This use case does not work as it generates client side JavaScript contetext.$control.Save() for the save button in the markup control. The Save property is not present in the $control context and attempt to press the "Save" button fails silently.

Since markup control properties of type Command generate no corresponding property into the $control client context no control commands can be called from staticCommand binding.

We need to correct this.

Copy link
Member

@exyi exyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still the problem that the translator does not know that the command invocation will return a promise and thus won't wait for it. It works for the simplest cases when only one command is invoked, but will not work when the command result value would be used in a second step, or so.

src/DotVVM.Framework/Controls/DotvvmMarkupControl.cs Outdated Show resolved Hide resolved
src/DotVVM.Framework/Controls/DotvvmMarkupControl.cs Outdated Show resolved Hide resolved
@quigamdev quigamdev requested a review from exyi January 28, 2021 22:10
@Mylan719
Copy link
Contributor Author

Mylan719 commented Jan 29, 2021

There is still the problem that the translator does not know that the command invocation will return a promise and thus won't wait for it.

This issue has been resolved, now the translator wraps the control property invocation result into Promise.resolve. It also adds correct annotation to the invocation so the compiler takes the property call into account when deciding the command execution order.

Copy link
Member

@exyi exyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thank you for this patch. Would be nice if you resolve my nitpicks.

We can add support for arguments later (ideally, with the lambdas merged), but I think that is going to be relatively easy.

@Mylan719
Copy link
Contributor Author

Mylan719 commented Feb 1, 2021

Nitpicks resolved ;)

Milan Mikuš added 2 commits February 2, 2021 17:41
…t it can be handled the same as any other property. Checking for MyCommand == null is now possible.

Added a sample with conditionaly hidden button.
@quigamdev quigamdev merged commit 757a217 into main Feb 17, 2021
@exyi exyi removed the proposal label Feb 21, 2021
@quigamdev quigamdev deleted the feature/staticCommand-in-markupControl branch February 26, 2021 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants