Refactor #19

tunnckoCore · 2018-11-14T08:30:53Z

Quite simplified.

Lodash and Handlebars (except for one specific case, need help) tests are working.

Sending the PR for now, but will continue work.

Also, do we need all the 0.3 methods? I think we can only rename resolve to resolveIds and make one more resolveId for single id.

Also, removed the devDeps (gulp) and updated to eslint config to latest found on engine refactor. All this was needed, because the config was pretty old and was throwing errors for async/await. But in anyway, you have update so you can update everything. I just did it to be able to start develop without frustrations and errors.

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore · 2018-11-14T08:35:32Z

test/handlebars.js

+  'lower(upper): {{lower (upper name)}}',
+  'spacer(upper, lower): {{spacer (upper name) (lower "X")}}',
+  'block: {{#block}}{{upper name}}{{/block}}',
+  'ifConditional1: {{#if (equals "foo" foo)}}{{upper name}}{{/if}}',


Pretty strange. I understand why it goes into the if. Because equals returns boolean false which is passed to the callback, but in reality, this means that it will get converted to string 'false', and that's why it goes inside.

Not sure how to handle this. You can see that the added ifConditional3 is working correctly and does to go inside the if.

I don't think we should handle such cases in the implementation, because it may break other cases such as lodash tests.

I think that in the equals helper we can check if it is falsey value then pass empty string or just call cb()

The reason this happens is because of the 2 steps that are taken... the template is rendered into a string with async ids, then the ids are resolved. Since the if helper isn't wrapped, the async id is passed directly to the if logic so it looks like a truthy value.

This is the same reason why in engines like lodash, you can't directly use if statements with async helpers.

It's something that we've talked about and think that it would take another pass at the "engine" level to possible detect built-in helpers (like {{#if}} in handlebars or control flow statements like if in lodash. I don't think it should be handled in async-helpers since this is intended to be template engine agnostic.

Aaaah. Yea, equals is async function so the return is again an id. ;d

I don't think it should be handled in async-helpers since this is intended to be template engine agnostic.

Agree.

But how then it is working in the current master (v0.3)? If it's not a problem, we can continue.

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore · 2018-11-14T08:44:22Z

test/handlebars.js

@@ -23,6 +23,8 @@ var tmpl = [
  'ifConditional1: {{#if (equals "foo" foo)}}{{upper name}}{{/if}}',
  'ifConditional2: {{#if (equals "baz" bar)}}{{upper name}}{{/if}}',
  'ifConditional3: {{#if foo}}{{upper name}}{{/if}}',
+  'ifConditional4: {{#if false}}{{upper name}}{{/if}}',


I don't get why when equals returns boolean false it does not work when this passes.

In the comment above, I explain how the async helper equals will actually return a string (the async id) during the rendering pass, then the id is resolved. Since false here is a literal value that handlebars handles, this works fine.

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore · 2018-11-14T09:52:40Z

Everything below Node < 8 fails.

doowb · 2018-11-14T15:06:30Z

test/test.js

+    // We just care and handle only async helpers here.
+    // The sync helpers are processed even on engine rendering,
+    // even before the `.resolve()` calling.
+    it.skip('should handle errors in sync helpers', function() {


The errors are caught and handled to add additional information to the error object. This is because not all template engines provide enough information about the error (like which helper the error occurred in).

Kinda make sense, yea. But the only thing that is a need that I can see is to wrap the sync helper in try/catch add meta info and rethrow.

doowb · 2018-11-14T15:18:25Z

@tunnckoCore thanks for this PR!

There are a few things that aren't handled now that were before (like resolving ids passed to sync helpers). I think this is because Jon and I had discussions about why that is necessary that probably didn't make it into comments.

I'd like to get some input from @jonschlinkert when he has a chance because we had talked about handling async helpers differently.

Thanks again for doing this and I'll try to give more feedback soon.

jonschlinkert

I'm not sure about this yet. I recently spent quite a bit of time on async helpers and took a different approach, you can see that here: https://github.com/jonschlinkert/templates/blob/refactor/lib/resolve.js. It seemed like there were a few things that weren't being handled in this lib, I would just want to make sure that we are handling partials, inline partials, and other edge cases that made this brittle before.

jonschlinkert · 2018-11-14T17:52:24Z

index.js

-function appendPrefix(prefix, counter) {
-  return prefix + counter + '$';
-}
+        const argz = args.map(function func(arg) {


can we use fat arrow here and this?

I'm using a regular one because later we map over the item.args, so to be recursive. And the context should be item.context.

jonschlinkert · 2018-11-14T17:52:42Z

index.js

-    var keys = Object.keys(helpers).filter(function(name) {
-      return ['async', 'sync', 'displayName'].indexOf(name) === -1;
+function promisify(fn) {
+  return function func(...args) {


we should use util.promisify

Yea, thought about that, but if someone wants to use it in the browser this way may save a lot.

That's the reason i didn't used the assert too.

tunnckoCore · 2018-11-14T19:29:58Z

@doowb: (like resolving ids passed to sync helpers)

Like {%= syncHelper(asyncHelper(foo)) %} kinda thing?
Hm. Some more real example in mind?

@jonschlinkert, agree. But I like that separation of engine, async-helpers, template and templates. Each one is on top of the other. And think that it's a good way async-helpers to handle only the helpers, and yes I agree to update it to wrap the sync helpers too, so we can make more meaningful errors. Then, on top of that will implement the partials

tunnckoCore · 2018-11-14T21:48:17Z

Yeah! Reimplemented it :D Works with sync, async and callbacks.

One question: do we need to support (foo, cb) => {}? It kinda requires to add one dependency :D Otherwise we'll end up with zero-deps.

Btw, preserved the API (mostly) ;p

.helper - set, get, getAll
.wrapHelper - set (wrap), get (return wrapped), getAll
.resolveId
.resolveIds

tunnckoCore · 2018-11-14T21:56:12Z

Couple of things to note:

you can directly define a helper with wrapHelper, instead of calling .helper first and then wrapping it. you still can get the not wrapped helper with helper(name) and the wrapped one with wrapHelper(name)
.helper() returns all helpers
.helper(name) returns the helper with name
.helper(name, fn) defines a helper
.helper(object) as usual, add each key/value pair
the .wrapHelper is exactly the same, with few notes:
- if there is not previously defined helper with .helper it defines it both as raw and wrapped
- if in .wrapHelper(name, fn) fn has fn.wrapped it directly returns it
- if helper with name cannot be found in both rawHelpers and wrappedHelpers it throws
- if sync or async helper (also wrapped and non wrapped) throw it notifies you which helper with what id and what's the message it throws

I have probably kidna weird work process :D
a) First write the code,
b) write examples (tweak the code if needed)
c) translate exampels to tests and in between write docs and transfer exampels into code comment blocks

It's definitely not TDD, probably BDD, but for sure it's weird. Or not. Maybe RDD.

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore · 2018-11-14T22:22:27Z

index.js

-        this.ids[id].value = await fn.call(context, ...(await Promise.all(argz)));
+      return id;
+    };
+    this.wrappedHelpers[name].wrapped = true;


Do we really need defineProperty / Reflect?

tunnckoCore · 2018-11-14T22:26:10Z

index.js

-const escapeRegex = require('escape-string-regexp');
-const isAsyncFunction = require('is-async-function');
+const util = require('util');
+const isAsyncWithCb = require('is-async-function');


I don't think we need to support callbacks. Users will be able to just pass the result of util.promisify(fn) as helper function ... Why add dependencies when we can't?

And if we switch to custom promisify we guaranteed friendly browser usage.

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore · 2018-11-14T22:59:26Z

Okey, Lodash is passing again. Handlebars fails in the useHash case (haha, the ifConditions are okey now).

Is that options.hash something Handlebars specific?

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

jonschlinkert · 2018-11-14T23:23:09Z

But I like that separation of engine, async-helpers, template and templates. Each one is on top of the other.

Yeah, the goal was to move the logic I did in templates over to this lib. But if you refactor works, that's fine too.

tunnckoCore · 2018-11-18T19:13:55Z

Is this API okey? So I can continue to the normal tests. :P

I think that one method that does both "set", "get" and "getAll" functionalities may be a bit confusing, but is also smaller code and kinda intuitive. Or making plural aliases too would make it less confusing?

jonschlinkert · 2018-11-18T20:21:30Z

It's definitely not TDD, probably BDD, but for sure it's weird. Or not. Maybe RDD.

lol, this had me laughing. I'm probably the same.

Is this API okey? So I can continue to the normal tests. :P

If it's not too much trouble, can you look through the helper tests in the templates refactor and see if everything is covered here?

But I like that separation of engine, async-helpers, template and templates. Each one is on top of the other.

That was the plan. I was going to move the async helper logic from templates to this lib before we released. I don't need that code to be used though, I appreciate you doing this!

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore · 2018-11-18T22:26:04Z

index.js

-      // }
+      if (isObject(arg) && isObject(arg.hash)) {
+        for (const key of Object.keys(arg.hash)) {
+          arg.hash[key] = await func(arg.hash[key]);


Absolutely intentionally using a recursive call to func() instead to the this.resolveId or this.resolveIds, because when the passed value is actual value and not an async id, then it (resolveId method) will throw.

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore · 2018-11-18T22:54:02Z

Okey, with the latest commit everything is a lot better. :D

I don't need that code to be used though, I appreciate you doing this!

Still think we can merge this one after porting the templates tests here and updating the current ones. It's pretty clear and easy to follow by others if they want to understand.

Haha, now I see why we can add .resolveArgs method too. :D

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore · 2018-11-19T00:51:42Z

test/test.js

@@ -179,6 +179,24 @@ describe('async-helpers', function() {
        assert.deepEqual(val, 'doowb');
      });
    });
+
+    it('should support sync and async value items in argument arrays', () => {


Might not be pretty clear what it does from the title...

But basically tests if array argument passed to helper, can have both actual values and asyncIds which will be resolved to actual values.

tunnckoCore · 2018-11-19T00:59:30Z

appveyor.yml


 # Install scripts. (runs after repo cloning)
 install:
  # Get the latest stable version of Node.js or io.js
  - ps: Install-Product node $env:nodejs_version
  # install modules
-  - npm install
+  - appveyor-retry npm install


appveyor-retry is to ensure it install everything properly. Because pretty weirdly some times it fails. Or at least it was that way before year or more.

It will run the command more than once ONLY if it fails (I believe).

tunnckoCore · 2018-11-19T01:07:40Z

Sweeet 🎉

I don't see what more. I think it is pretty enough and stable. Please try to integrate it that way first. And if something is not as expected, then throw it in the garbage and do whatever you want. But I have strong confidence in it. I soon may add the tests from templates too.

The templates implementation is basically the same, plus trails from that that it is tightly coupled with the rest of the things there (e.g. this.app, ctx.app and etc). In any way, it not seems so clean and easy to follow as this one here.

Charlike Mike Reagent added 4 commits November 14, 2018 09:06

chore: cleanup

9808df3

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

fix: add source, update deps & eslint config

a7d2744

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

fix: start tests

a3511bc

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

fix: make it work for hbs and helpers context

7d3efbe

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore commented Nov 14, 2018

View reviewed changes

fix: more ensurance cases

869210a

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore commented Nov 14, 2018

View reviewed changes

Charlike Mike Reagent added 4 commits November 14, 2018 10:55

fix: hbs tests

55c8546

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

fix: start updating the regular tests

11a9b49

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

fix: rename .helper -> .set

46c1bd6

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

fix: 100% coverage

b8e4132

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore mentioned this pull request Nov 14, 2018

Async helpers support jonschlinkert/engine#6

Open

doowb reviewed Nov 14, 2018

View reviewed changes

jonschlinkert reviewed Nov 14, 2018

View reviewed changes

rewritten again, a lot better

9b09577

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore commented Nov 14, 2018

View reviewed changes

start updating tests

ed58a0c

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

fix examples/refactor.js, add helper returning a promise

c681b8e

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

fix: finally working with both lodash and hbs

a1bdc53

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore commented Nov 18, 2018

View reviewed changes

fix: add .hasAsyncId method and so better args resolving

d94cca1

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

Charlike Mike Reagent added 4 commits November 19, 2018 01:45

fix: adapt rest of the tests

7c782bd

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

fix: back to 100% coverage

7a7536a

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

fix: drop support for Node < 8

377205f

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

fix: appveyor test on node >= 8

fda97ea

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>

tunnckoCore commented Nov 19, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor #19

Refactor #19

tunnckoCore commented Nov 14, 2018 •

edited

tunnckoCore Nov 14, 2018

tunnckoCore Nov 14, 2018

doowb Nov 14, 2018

tunnckoCore Nov 14, 2018 •

edited

tunnckoCore Nov 14, 2018

doowb Nov 14, 2018

tunnckoCore commented Nov 14, 2018

doowb Nov 14, 2018

tunnckoCore Nov 14, 2018

doowb commented Nov 14, 2018

jonschlinkert left a comment

jonschlinkert Nov 14, 2018

tunnckoCore Nov 14, 2018

jonschlinkert Nov 14, 2018

tunnckoCore Nov 14, 2018

tunnckoCore Nov 14, 2018

tunnckoCore commented Nov 14, 2018

tunnckoCore commented Nov 14, 2018

tunnckoCore commented Nov 14, 2018 •

edited

tunnckoCore Nov 14, 2018

tunnckoCore Nov 14, 2018 •

edited

tunnckoCore Nov 14, 2018

tunnckoCore commented Nov 14, 2018

jonschlinkert commented Nov 14, 2018

tunnckoCore commented Nov 18, 2018

jonschlinkert commented Nov 18, 2018

tunnckoCore Nov 18, 2018

tunnckoCore commented Nov 18, 2018

tunnckoCore Nov 19, 2018 •

edited

tunnckoCore Nov 19, 2018 •

edited

tunnckoCore commented Nov 19, 2018

Refactor #19

Are you sure you want to change the base?

Refactor #19

Conversation

tunnckoCore commented Nov 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunnckoCore Nov 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunnckoCore commented Nov 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doowb commented Nov 14, 2018

jonschlinkert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunnckoCore commented Nov 14, 2018

tunnckoCore commented Nov 14, 2018

tunnckoCore commented Nov 14, 2018 • edited

Choose a reason for hiding this comment

tunnckoCore Nov 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunnckoCore commented Nov 14, 2018

jonschlinkert commented Nov 14, 2018

tunnckoCore commented Nov 18, 2018

jonschlinkert commented Nov 18, 2018

Choose a reason for hiding this comment

tunnckoCore commented Nov 18, 2018

tunnckoCore Nov 19, 2018 • edited

Choose a reason for hiding this comment

tunnckoCore Nov 19, 2018 • edited

Choose a reason for hiding this comment

tunnckoCore commented Nov 19, 2018

tunnckoCore commented Nov 14, 2018 •

edited

tunnckoCore Nov 14, 2018 •

edited

tunnckoCore commented Nov 14, 2018 •

edited

tunnckoCore Nov 14, 2018 •

edited

tunnckoCore Nov 19, 2018 •

edited

tunnckoCore Nov 19, 2018 •

edited