fix: use the dust context which is passed in arg

[kumarrishav]

In node 8, when using factory(require('dustjs-linkedin'), it seems it creates a new dust context/object and assign dust.helpers to it, because of which actual dust.helpers was {}. So we were loosing all the dust-helpers.

Ideally it should refer to same cached dustjs-linkedin object, but it's not happening. Also, i didn't understand why we are requiring it separately when it's is already expecting a function and passing dust in arg i.e module.exports = function(dust, options) {} // for dust helpers.

Solution is: use the dust object coming as arg instead of requiring it again as require won't get it from cache because of cache clean by freshy.

[hegdeashwin]

Superb! This is working as expected.

This will solve issue linkedin/dustjs#776

[jimmyhchan]

For compatibility reasons, I would recommend not modifying the tests. It looks like adding node 4, 6, 8 to the travis test the existing tests pass.

It seems related to how the the end developer requires dustjs-helpers. Specifically:

// BTW - this is what the existing tests are doing
var dust = require('dustjs-linkedin');
require('dustjs-helpers');

VS

var dust = require('dustjs-helpers');

This discussion might also be helpful #72.

[kumarrishav]

@jimmyhchan yeah, i checked that discussion. It's almost similar i.e issue is similar but this time reason is different. Helper is working fine upto version node6, but not in node 8. That's why i added that fix to support all the version v4/v6/v8.

I changed the test version as node 0.x is outdated and doesn't have support

[jimmyhchan]

Please consider a different approach here. To be specific changing the return type to become a factory that produces a dust instance is a very large breaking change and will not be pulled in. Also please consider providing an example (ideally a test case) where this is failing in node 8. With a failing test case and keeping all existing tests as is, we can discuss possible options instead of a major breaking api change. It’s very possible I don’t have a clear idea what the issue is.

[kumarrishav]

@jimmyhchan agree it will be a bigger change.
How about this approach? In this case it won't break existing behaviour too.

    module.exports = factory(require('dustjs-linkedin'));
    module.exports.registerWith = factory;  

https://github.com/krakenjs/adaro/blob/v1.x/lib/engine.js#L120
https://github.com/krakenjs/adaro/blob/v1.x/lib/reqwire.js#L60

[grawk]

@jimmyhchan.. @kumarrishav 's latest suggestion appears to be non-breaking, and keeping with the convention of some other helpers to export a registerWith method. This would unblock a lot of our applications in their migration to node@8.

[jimmyhchan]

@grawk @kumarrishav Agreed this can be added as a undocumented workaround for now.

Some blockers:

• Please provide a baseline travis run with the addition of node 4, 6, 8 (please do not remove 0.12 and 0.10). I would like confidence that A) node 8 is indeed failing without this fix (i'm still not able to reproduce this locally) B) the test failure from travis https://travis-ci.org/linkedin/dustjs-helpers/builds/313097608?utm_source=github_status&utm_medium=notification is not related to the code change (should not). Dropping 0.10 and 0.12 support is fairly easy to do but that's a major Semver change (IMO)
• Please add the registerWith API as noted above.

[kumarrishav]

@jimmyhchan added the suggested change. But some weird error is coming in test (test didn't even start running). Not sure how template string is coming here. Seems like part of npm but why test would effect that? also, 0.x doesn't have support for template string

[kumarrishav]

after searching a bit, i found these npm/npm#18963. It says, because of node 0.x which is not supported anymore.

https://stackoverflow.com/questions/46852889/unable-to-change-node-version-using-nvm

[jimmyhchan]

Thanks for all your help here! Hope to push a patch out in the next little bit

[jimmyhchan]

1.7.4 is now published. #147 to track the fix failing tests while also killing 0.10 and 0.12 support

[kumarrishav]

Thanks @jimmyhchan

[grawk]

Thanks @jimmyhchan and @kumarrishav !!