Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

then warning creates messy logs for thenable APIs #482

Closed
bendrucker opened this issue Feb 4, 2015 · 12 comments
Closed

then warning creates messy logs for thenable APIs #482

bendrucker opened this issue Feb 4, 2015 · 12 comments

Comments

@bendrucker
Copy link

@bendrucker bendrucker commented Feb 4, 2015

The recent addition of warnings when then is called with a non function argument is creating messy test output over at Knex. The basic issue here is that query objects are thenables that proxy to a real promise which gets created. Since Promise.longStackTraces turns on debugging, we're getting loads of warnings about empty thens in the integration tests.

The easy fix here is just to revert the warning, but I understand if there was a compelling reason to do it. We could also use promise.then.apply(promise, arguments) in place of explicitly passing the two callbacks to promise.then. I'd prefer not to go that route since it's harder to read/understand.

@myndzi
Copy link

@myndzi myndzi commented Feb 4, 2015

I don't see how this is a bluebird problem in any way. The warning is justified, and knex is doing weird things it shouldn't. The fix is to stop doing weird things, not to complain to bluebird to remove the warnings. I've NEVER had good results trying to wrap or fiddle with the "then" method of a promise, or providing a pretend/pseudo promise by providing a then method. I don't understand what knex is trying to do well enough to suggest a viable alternative, but I can see that it's doing something that will only cause pain.

Edit: possibly the hooks patch may provide a viable alternative to "doing something after the query"

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Feb 4, 2015

@myndzi I totally see why this is our problem. We have every obligation not to break backwards compatibility and I'm against any reasoning that blames the users of the library for a breaking change.

When we introduced the warnings we could not come up with reasonable workflow issues (We considered cloning a promise).

@bendrucker there will be a hook that allows you to opt-out of these warnings (but it's planned for 3.0 for better debuggability). I'm afraid it's not there yet - I wonder if it can be backported to 2.9.*

In the interim I suggest a revert in 2.9.x - what do you think @petkaantonov ?

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Feb 4, 2015

@bendrucker why would you .then() a transaction or a target() without arguments?

@myndzi
Copy link

@myndzi myndzi commented Feb 4, 2015

It's not a breaking change though?

The code is apparently calling .then() to get an instance of a promise derived from the source (where the '.then' method had been replaced); it seems to want to execute some code before executing the .then handler, but it doesn't want to execute that code until the .then method is called, thus the whole wrapping thing.

@bendrucker
Copy link
Author

@bendrucker bendrucker commented Feb 4, 2015

@benjamingr A hook sounds perfect. There are a number of cases where you might end up calling then without arguments. One is if you want to get at a Bluebird helper method that's not proxied, e.g. query.then().get(0). Another would be returning inside a promise:

promise.then(function () {
  return query;
});

So query.then is implicitly called with no arguments in that case. This totally goes away when applying arguments from the thenable's then to the actual promises's then instead of manually passing them.

In general I'm of the opinion that warnings should always be opt-in and libraries should throw when something's wrong and never write to the console. So in this case my ideal solution (not Knex's, but the one that fits best with how I write things) would be to change the philosophy around warnings. A user could opt in, a la strict mode, to have Bluebird throw in cases like this.

@bendrucker
Copy link
Author

@bendrucker bendrucker commented Feb 4, 2015

Also this is purely a cosmetic issue right now with the test logs. It's not going to affect users of Knex.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 4, 2015

@bendrucker You could just call ._then? It's forced to retain same api signature across all versions, even major.

btw In 3.0 warnings are disabled/enabled with

Promise.config({
     warnings: false,
     longStackTraces: true
});

Note that .then() doesn't cause a warning, in fact it's used internally many times. Warning is only given when you explicitly pass value(s) that are all non-functions.

@bendrucker
Copy link
Author

@bendrucker bendrucker commented Feb 4, 2015

If you're considering reverting and saving for 3.0 I'll wait on that. Otherwise I'll drop in a fix and probably just end up applying the arguments.

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Feb 4, 2015

@bendrucker note that you did opt into the debug build when you enabled long stack traces. Like petka said in v3 we'll have finer grained control.

I'm for reverting this and introducing it in a major for breaking workflows.

bendrucker added a commit to knex/knex that referenced this issue Feb 7, 2015
@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Feb 8, 2015

@petkaantonov any update on reverting this in 2.x? There has been a release since the issue opened and I want to know if you decided one way or the other.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 8, 2015

Well I don't see any reason to revert this, the fix is simple for knex and people will use 2.x for a while.

@bendrucker
Copy link
Author

@bendrucker bendrucker commented Feb 8, 2015

Fair enough. I've fixed it in knex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants