airbnb / hypernova

A service for server-side rendering your JavaScript views
MIT License
5.82k stars 212 forks source link

[Fix] Add return in Promise 'then' callback #41

Closed kenju closed 7 years ago

kenju commented 7 years ago

return null in Promise then callback to suppress Bluebird warning.

Though this is not required by ES6 Promise specification, this repo should comply to Bluebird best practice unless using it. Otherwise redundant log is printed out into console in development, which is annoying and have nothing to do with hypernova or its users:

screen shot 2017-01-19 at 10 40 03 am

Documents

From Bluebird: http://bluebirdjs.com/docs/warning-explanations.html#warning-a-promise-was-created-in-a-handler-but-was-not-returned-from-it

If you know what you're doing and don't want to silence all warnings, you can create runaway promises without causing this warning by returning e.g. null:

getUser().then(function(user) {
// Perform this in the "background" and don't care about its result at all
saveAnalytics(user);
// return a non-undefined value to signal that we didn't forget to return
return null;
});
kenju commented 7 years ago

@ljharb Thank you for reviewing.

@goatslacker I would appreciate if you take time to review this PR 🙏

ljharb commented 7 years ago

@kenju would you mind rebasing on the command line, so there's no merge commits in the PR?

kenju commented 7 years ago

@ljharb Sure! I did ( also squashed first commit, which was just returning null, into one commit).

goatslacker commented 7 years ago

So we don't change the semantics can we just return undefined from each?

ljharb commented 7 years ago

@goatslacker the warning is because the promises being created are being thrown away - the best practice is to let every promise be consumed by something. Is there a problem with returning a promise here?