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: fix parameter passing (in async commands) #1170

Merged
merged 6 commits into from Oct 20, 2021

Conversation

exyi
Copy link
Member

@exyi exyi commented Oct 14, 2021

The parameters were passed in a very weird way, now it works similar
to command bindings - even when command is not a lambda function, it is
wrapped in a lambda function. Then, the lambda function gets parameters
assigned directly, so the lambda syntax is essentially eliminated.
This back and forth is kinda needed to align how it works in case command
is specified as plain expression and when it's specified as
lambda function.

resolves #980

@exyi exyi force-pushed the bug/parameter-lambda-static-command-services branch from 206c416 to 228cf5d Compare October 14, 2021 21:57
@@ -351,7 +351,7 @@ public DataSourceLengthBinding GetDataSourceLength(ParsedExpressionBindingProper
}


public StaticCommandJsAstProperty CompileStaticCommand(DataContextStack dataContext, ParsedExpressionBindingProperty expression) =>
public StaticCommandJsAstProperty CompileStaticCommand(DataContextStack dataContext, CastedExpressionBindingProperty expression) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually one of the more important changes - the CastedExpression is automatically wrapped in a lambda function unless it's already a delegate. We already have this logic for commands, now we also use it for static commands

@@ -154,7 +154,7 @@ export async function applyPostbackHandlers(

try {
const commit = await applyPostbackHandlersCore(saneNext, options, handlers);
const result = await commit(...args);
const result = await commit();
Copy link
Member Author

Choose a reason for hiding this comment

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

We were passing arguments through the commit function, which should not even be used in static commands 🤦 I'm surprised it did not create any magic problems with postback handlers

@exyi exyi changed the title @exyi static commands: fix parameter passing (in async commands) static commands: fix parameter passing (in async commands) Oct 14, 2021
tomasherceg and others added 2 commits October 16, 2021 12:51
The parameters were passed in a very weird way, now it works similar
to command bindings - even when command is not a lambda function, it is
wrapped in a lambda function. Then, the lambda function gets parameters
assigned directly, so the lambda syntax is essentially eliminated.
This back and forth is kinda needed to align how it works in case command
is specified as plain expression and when it's specified as
lambda function.

resolves  #980, #1165
@exyi exyi force-pushed the bug/parameter-lambda-static-command-services branch from 228cf5d to ca2ab8a Compare October 16, 2021 12:53
@quigamdev quigamdev requested review from acizmarik and quigamdev and removed request for acizmarik October 16, 2021 13:40
Copy link
Member

@acizmarik acizmarik left a comment

Choose a reason for hiding this comment

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

Good fix 👍. Just please have a look at the comments, after that we can merge right-away.

src/Tests/Binding/StaticCommandCompilationTests.cs Outdated Show resolved Hide resolved
@@ -49,6 +49,10 @@ public static Expression Replace(LambdaExpression ex, params Expression[] parame

#region Replace overloads

public static Expression Replace<TRes>(Expression<Func<TRes>> ex)
Copy link
Member

Choose a reason for hiding this comment

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

Although it is not really in scope of this PR, I think that ReplaceVisitor is not that good name. From this method it is not clear what gets replaced. If I understood correctly it patches stubs and replaces parameters. To me it looks more like a "method patching visitor" or something.

Might be worth refactoring if we can come up with a more descriptive name both for the visitor and for these methods.

@exyi exyi merged commit ce3ceeb into main Oct 20, 2021
@exyi exyi deleted the bug/parameter-lambda-static-command-services branch October 20, 2021 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lambda in staticCommand can not pass arguments to server methods
3 participants