GH-32 - Use iteration to populate dust.helpers

[mouyang]

GH-32 - Instead of overwriting dust.helpers in the module, use iteration to populate the header methods. This should be fine because dustjs <--dependsOn-- dustjs-helpers and dustjs defines a helpers attribute.

• Why you decide to implement this feature.

This seems like a long standing issue that is not immediately obvious from the documentation. I use custom helpers in my private project and I could have encountered this issue - I didn't only because how I set up my dependencies in require.js (custom helpers were in an AMD depending on full dust.js).

• In which cases it will be useful.

Any time helpers get called between dustjs and dustjs-helpers declarations.

The main roadblock seems to be race conditions, so I wanted to force a specific script loading order. I used LABjs to do this on the client, and it was a code rearrangement on the server.

• Side effects

If user defines helpers with the same names defined in dustjs-helpers, the last declaration will win. I have no idea how to handle that beyond namespaces within dust.helpers, or declaring caveat emptor. That seems clunky and that would probably require more widespread code changes.

[kate2753]

Could you please explain what is specRunner2.js and how it's different from specRunner?

[mouyang]

I can. It's a bit hard to pick up. The server specRunners are almost identical and the difference is very subtle.

The main difference is the in the order of the assignment statements. In specRunner it goes dust->dust.helpers->dust.helpers.tapper, whereas in specRunner2 it goes dust->dust.helpers.tapper->dust.helpers.

Both specRunners run the same battery of tests and thus should produce consistent results. However without the fix in dust-helpers.js, specRunner will pass all tests but specRunner2 will not because dust.helpers.tapper gets clobbered inside dust-helpers.js.

In summary, specRunner2.js simulates the assignment order @rragan described in the ticket I referenced.

$ diff -u specrunner.js specrunner2.js
--- specrunner.js       wed jan 15 16:44:37 2014
+++ specrunner2.js      thu jan 16 22:50:51 2014
@@ -4,15 +4,14 @@

 /* this should be declared global in order to access them in the spec*/
 dust = require('dustjs-linkedin');
-dust.helpers = require("../../../lib/dust-helpers").helpers;
-helperstests = require('../spec/helperstests');
-
 //add the tapper helper to test the tap helper.
 dust.helpers.tapper = function(chunk, context, bodies, params) {
   var result = dust.helpers.tap(params.value,chunk,context);
   chunk.write(result);
   return chunk;
 };
+dust.helpers = require("../../../lib/dust-helpers").helpers;
+helperstests = require('../spec/helperstests');

[kate2753]

Thanks for the explanation. I feel that updating existing spec runner with new order of require statements should be sufficient. We probably do not need a separate spec runner just to test this.

The change to dust-helpers.js file looks good to me.

[mouyang]

Yeah, it was admittedly overkill. I cleaned it up.

[kate2753]

Client side spec runner has changed when we switched to grunt build. It is now auto generated. You'll need to do git pull to get latest changes. Your changes to server side spec runner still apply.

Take a look at the jasmine task config in gruntfile.js on line 85. You'll need to play with the order of files in config. If you want to see the generated spec runner - change keepRunner option in config to true. Next time you run grunt testPhantom file _SpecRunner.html will be saved in the root directory of the project.

Also, is adding LAB.src.js absolutely necessary? If so, I would rather add it as a proper node dependency into package.json and reference from node_modules folder instead of checking in into the project.

[mouyang]

I'll take a look at this when I get the chance.

LAB.src.js is there for the same reason the previously removed server-side specrunner duplicate - to enforce a specific script-loading order that creates an error. I do agree about using a proper node dependency, but I couldn't find it in the npm repository.

That said, I think we can just revert the client-side spec runner like you suggested for the server-side. It is kind of overkill.

[mouyang]

Hi Kate. Per your suggestion, I removed the old client specrunner and the LAB.src.js file. If you have any other suggestions, please let me know.

[kate2753]

dust.helpers.tapper was moved to testUtils so it's easier to maintain in across Phantom, Rhino and Node tests.

Instead of adding tapper helper back here you can simply do

//This should be declared global in order to access them in the spec
dust = require('dustjs-linkedin');

require("../../../test/testUtils");
require("../../../lib/dust-helpers");

//Get unit test specs
helpersTests = require('../spec/helpersTests');

This order should test your use case.

Can you also squash the commits? We'll be ready to pull in your change after this update

[kate2753]

This was resolved with #73. Closing this pull request. Please re-open if you think I've missed anything.