Add warning when using .returning() in sqlite3. Fixes #1660

[wubzz]

No description provided.

[elhigu]

hmm.. for some reason some tape oracle tests are running with older nodes, which shouldn't even test for oracle (node-oracledb driver stopped providing binaries for older node versions). I've never seen those fails before... trying to rerun

[wubzz]

@elhigu Yeah been trying to rerun these the past two hours. It doesn't make much sense :/

[elhigu]

Maybe they have again done some changes or something in the driver which breaks the tests.

If you don't have hurry with this. I can figure out the cause of breakage during next week. Probably all the other new PRs will be failing too.

[wubzz]

@elhigu Seems oracledb module requires node 8+ according to their latest change -- this utilizes stream.destroy() which according to node docs is not available until 8+. Very strange how Node 5 is passings tests, tho. Perhaps the event is never emitted there?

I'll leave this PR open, its no rush.

[elhigu]

Node 5 and 7 should not be running oracle tests at all, because driver couldn't be installed there anymore. But 4 and 6 were still supposed to be supported while ago.

[elhigu]

Oracle says that it should support node 4,6,8,9 so that self.destory() sounds like node-oracledb bug. Good catch 👍

[wubzz]

I created an issue oracle/node-oracledb#847

[elhigu]

Meanwhile if tests are passing and only error are those oracle ones, we can merge there PRs

[faustbrian]

Is there some way to disable this warning? It clutters the terminal quite a bit while seeding and debugging and makes it impossible to read anything else during development.

Currently just commented it out in the node_modules files.

[elhigu]

At least it is possible by not using .returning() with sqlite. Also it can be disabled with grep - stdout. There is huge demand for a feature, which will allow hooking in own logging implementation. Ultimately that will allow to filter all logs outputted by knex.

[faustbrian]

Yeah, that would be great to hook something like winston up. Was just wondering why that message spammed my screen as I am not calling that method. (maybe objection or bookshelf do)

[elhigu]

yeah probably objection and bookshelf are using that and both libs are probably using it correctly 👍

[mceachen]

I've been burned by both winston and bunyan showstopper bugs--please don't pull either of those in as deps. I don't feel that requiring users to grep -v stdout or stderr is a reasonable solution either.

Would it be OK to only omit the warning once?

Another solution, could you not omit the warning at all if, say, the KNEX_SUPRESS_SQLITE_RETURNING_WARNING env was set, or, perhaps better, something like the useNullAsDefault config flag was added, perhaps?

[wubzz]

I much rather merge the WIP branch by @tgriesser if he feels that it is done. I don't like adding temporary deviations.