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

Await tag #312

Merged
merged 9 commits into from
Jun 24, 2016
Merged

Await tag #312

merged 9 commits into from
Jun 24, 2016

Conversation

mlrawlings
Copy link
Member

This:

<async-fragment data-provider=data.userDataProvider var="user">
    Hello, ${user.name}
</async-fragment>

becomes this:

<await(user from data.userDataProvider)>
    Hello, ${user.name}
</await>

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 and data-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.

@coveralls
Copy link

coveralls commented Jun 18, 2016

Coverage Status

Coverage increased (+0.3%) to 83.728% when pulling b3c3c56 on mlrawlings:await-tag into 74120db on marko-js:master.

@mlrawlings
Copy link
Member Author

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 await being used as a variable/function name.

We could probably utilize a list of reserved words and append Tag to custom tags that match an item in that list. In this case we'd have awaitTag({}, out).

@patrick-steele-idem
Copy link
Contributor

Very nice and much cleaner. Thanks for working on this! The only thing that kind of bothers me is the from keyword...it works, but just want to make sure we explore all syntax options, what are your thoughts on reversing the syntax?:

<await(data.userDataProvider as user)>
    Hello, ${user.name}
</await>

I read that as "await on data.userDataProvider and store the result as the user variable". It also is a little closer to the actual JavaScript syntax where await is in front of the expression that produces the Promise:

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 ');
Copy link
Contributor

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);

Copy link
Member Author

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.

@mlrawlings
Copy link
Member Author

Both make sense when you read them as a sentence:

await user from theUserDataProvider
await theUserDataProvider as user

from has the benefit of the variable name being to the left of the statement, which is closer to for loops and assignments, where as as has the benefit, as you said, of keeping await closer to the provider that it is awaiting which is how it's done in JS.

I'm kinda leaning towards from, but it may be just 'cause it's what I've had in my head while thinking about this... what does @tindli, @philidem, and the rest of the community think?

as is two characters shorter than from...

@kprakasam
Copy link
Contributor

When I read await theUserDataProvider as user, I thought I am creating an alias user for theUserDataProvider, where as await user from theUserDataProvider seem to make the intent clear.

@mlrawlings
Copy link
Member Author

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 userDataProvider is just an means to an end. The only reason I care about the userDataProvider is that it will eventually give me the user. The user is what I really want. I am awaiting the user.

If I could, I'd simply write

<await(user)>

But that doesn't give enough information. <await> doesn't know where the user comes from.

<await(user from userDataProvider)>

Ah. Okay, it comes from the userDataProvider. Now <await> can get me my user.


Also, all feedback has either been neutral or in favor of the from syntax.

Let's go with from.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 83.728% when pulling ef29452 on mlrawlings:await-tag into 74120db on marko-js:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 83.728% when pulling ef29452 on mlrawlings:await-tag into 74120db on marko-js:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 83.728% when pulling ef29452 on mlrawlings:await-tag into 74120db on marko-js:master.

@patrick-steele-idem
Copy link
Contributor

@mlrawlings

Let's go with from.

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 marko installed to determine how the code should be parsed and migrated.

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.

4 participants