-
Notifications
You must be signed in to change notification settings - Fork 644
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
Await tag #312
Await tag #312
Conversation
…deprecated events, tests, and transforms)
I forgot to mention: Currently, the output JS looks something like this: //...
await = __loadTag(require("../../../../taglibs/async/await-tag"));
//...
await({
dataProvider: data.userDataProvider,
_name: "data.userDataProvider",
renderBody: function renderBody(out, user) {
out.w("<div class=\"foo\">Hello, "+escapeXml(user.name)+"</div>");
}
}, out);
//... and while this actually works, I'm not thrilled about We could probably utilize a list of reserved words and append |
Very nice and much cleaner. Thanks for working on this! The only thing that kind of bothers me is the <await(data.userDataProvider as user)>
Hello, ${user.name}
</await> I read that as "await on var user = await data.userDataProvider; (of course, in JavaScript the variable is on the left hand side...) I could probably go either way, but just wanted to throw the alternate option out there. What do you think? |
|
||
varName = varName.value; | ||
var parts = el.argument.split(' from '); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using split is not a good idea since there is very small chance from
might appear in the expression. I would use a regular expression that is rooted on the side of the variable name to separate out the variable name from the potentially complex expression:
var matches = / from ([$A-Z_][0-9A-Z_$])$/i.exec(argument);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I was thinking that split with a string argument would only split on the first instance it found, but that's definitely not the case. Once we figure out as
vs from
I'll change it to use a rooted regex.
Both make sense when you read them as a sentence:
I'm kinda leaning towards
|
When I read |
So after thinking about this a bit more, I've come to the conclusion that I'm not so much awaiting the promise, because I already have the promise. I'm awaiting what I get when the promise is fulfilled. In our example, the If I could, I'd simply write <await(user)> But that doesn't give enough information. <await(user from userDataProvider)> Ah. Okay, it comes Also, all feedback has either been neutral or in favor of the Let's go with |
2 similar comments
Sounds good to me. I'll be spending a little more time reviewing and will publish a new version soon. In the meantime, would you be interested in updating the migration tool to migrate to the new await syntax from Marko v2? We should also consider making the migration tool a little smarter to look at the version of |
This:
becomes this:
Tag changes:
<async-fragment>
=><await>
<async-fragments client-reorder/>
=><await-reorderer/>
<async-fragment-placeholder>
=><await-placeholder>
<async-fragment-timeout>
=><await-timeout>
<async-fragment-error>
=><await-error>
A transform is provided that will transform the async-fragment versions of these tags to the await version, so nothing will actually break, but a deprecation message will be logged to the console during template compilation.
This is primarily a syntax change. All tag attributes remain the same, except that
var
anddata-provider
are now specified in the<await>
tag's argument.Event changes:
asyncFragmentBegin
=>await:begin
asyncFragmentBeforeRender
=>await:beforeRender
asyncFragmentFinish
=>await:finish
asyncFragmentClientReorder
=>await:clientReorder
The async-fragment versions of these events are still emitted, so as to not immediately break modules that make use of these events. Those libraries should begin to migrate as we'll likely remove the duplicate events at some point.