async controller fails to bind to routes

[adnan-kamili]

BEGIN VERSION INFO

-->
Sails version: 0.12.7
Node version: 7.0.0
NPM version: 3.10.8
Operating system: macOS Sierra

[sailsbot]

Hi @adnan-kamili! It looks like you may have removed some required elements from the initial comment template, without which I can't verify that this post meets our contribution guidelines. To re-open this issue, please copy the template from here, paste it at the beginning of your initial comment, and follow the instructions in the text. Then post a new comment (e.g. "ok, fixed!") so that I know to go back and check.

Sorry to be a hassle, but following these instructions ensures that we can help you in the best way possible and keep the Sails project running smoothly.

If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact inquiries@treeline.io.

[adnan-kamili]

ok fixed

[karsasmus]

Sails uses async as dependency. Perhaps this causes your error?

[adnan-kamili]

Check the updated issue perhaps it is a bug in underscore lib

[adnan-kamili]

To fix the issue I made following change in the sails core - sails/lib/hooks/controllers/to-interpret-route-syntax.js:

//if ( !_.isFunction(originalFn) ) { - buggy line

  if ( typeof originalFn !== 'function' ) {
    sails.after('lifted', function () {
      sails.log.error(
        'In '+controllerId + '.' + actionId+', ignored invalid attempt to bind route to a non-function controller:',
        originalFn, 'for path: ', path, verb ? ('and verb: ' + verb) : '');
    });

But routes are still not binding

[mikermcneil]

Hey @adnan-kamili, out of curiosity, why do you need to be able to write async function()? I'm not sure what benefit it affords, since controller actions are always invoked by Sails and should never be called directly.

[adnan-kamili]

Making the controller function async makes it possible to await the Promises inside the controller. So I can avoid callbacks and promise then chains:

let item = await Items.findOne(req.params.id);
// then do something with item

instead of

Items.findOne(req.params.id)
.then(function(item){
})
// and the chain continues

[mikermcneil]

@karsasmus good thinking btw -- could easily be related (cc @megawac)

@adnan-kamili ah gotcha-- I didn't realize this was limited to functions marked with the async keyword. We use _.isFunction() as a best practice throughout the Sails code base and related modules, so I don't want to change that right now. But I'll def. look into this further-- thanks for following up!

[mikermcneil]

@karsasmus ohhh nevermind, apologies. (@megawac, sorry to bother you)

It's lodash/lodash#2768

[mikermcneil]

@karsasmus @adnan-kamili Posted here: lodash/lodash#2768 (comment)

@sailsbot including you too, just for the moral support

[adnan-kamili]

@mikermcneil The bug replicates for arrow functions too in controller even without using async keyword.

[adnan-kamili]

For anyone looking for a safe hotfix until a fix lands in the sails.js (most probably requiring a fork of lodash@3) you can override the lodash object in following file:

sails/lib/hooks/controllers/to-interpret-route-syntax.js

var _ = require('lodash');
_.isFunction = function(value) {
    return typeof value === "function" 
  };

[mikermcneil]

for arrow functions too

@adnan-kamili gotcha, thanks

For anyone looking for a safe hotfix until a fix lands in the sails.js

Thanks for posting that! I'll push out a fix for this on 0.12.x asap.

BTW, while this is not exactly relevant to this issue, since it brings it to mind: In Sails v1, you'll get your own install of lodash and async by default. (So that way, when you're using globals, your apps won't be relying on the internal implementation details of Sails insofar as the version of Lodash we're using.)

[mikermcneil]

@adnan-kamili ok I've got this working locally with a Lodash 3.10.2 patch on a branch-- waiting to hear back from someone and then will post back here to ask you to confirm. Thanks again for the help!

[adnan-kamili]

@mikermcneil Waiting for the fix

[mikermcneil]

@agnan-kamili k it's on master (lodash/lodash#2768 (comment))

[adnan-kamili]

@mikermcneil master seems to be at v1.0.10. Can you do an release on 0.12.x_rc so that I can check it out.

[adnan-kamili]

@mikermcneil when can we expect the fix to land in 0.12.x branch

[mikermcneil]

@adnan-kamili Thanks for the reminder! Backporting this isn't at the top of my list right now tbh, since it's not affecting documented usage or existing code in 0.12 apps-- although I am 100% down to backport support for node 7 async functions to a Sails 0.12 release ASAP! I'll just need someone to help test it out on master first.

[adnan-kamili]

@mikermcneil I have tested it with sails v1.0.0-11 and it is working fine.

• No errors were logged
• Endpoints are working fine (routes are binding)
• req.params object is getting populated correctly

[mikermcneil]

@adnan-kamili thank you! I'll backport this for v0.12 today

[mikermcneil]

@adnan-kamili just published under the "0.12-pre" tag:

npm install sails@^0.12.11-0 --force

It's working for me in the example app from the book, but just as a final verification, would you give that a go and give me the thumbs up if it works for you? Then I'll go ahead and publish 0.12.11 (in the mean time, you can change your package.json to ^0.12.11-0 and it should automatically pick up the proper version from NPM as soon as it's published as "latest"). Thanks!

[adnan-kamili]

I tested it and it works fine :)

Thanks for backporting it.

[mikermcneil]

@adnan-kamili thank you! Bundling one other bug fix and publishing as 0.12.11 today, hopefully. In the mean time, your semver range should continue to work indefinitely 👍

[mikermcneil]

@adnan-kamili Just a heads up: This is now published as latest (0.12.11).

[adnan-kamili]

@mikermcneil Thanks :)