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

Fix event listener duplication #2982

Merged
merged 6 commits into from Jan 31, 2019

Conversation

Projects
None yet
2 participants
@kibertoad
Copy link
Collaborator

kibertoad commented Jan 5, 2019

fixes #2967
Note that something works very differently in Node 6 and this causes removal of duplicate event listeners to result in also mutating list of event listeners in the original Knex instance, which is a much more serious concern than the problem this PR aims to fix.
This PR doesn't make functionality on Node 6 any more broken than it already was, and for newer Node version it seems to be fixed.

@kibertoad kibertoad requested a review from tgriesser Jan 5, 2019

kibertoad added some commits Jan 5, 2019

@@ -31,6 +31,8 @@ describe('Query Building Tests', function() {
require('./unit/schema/oracledb');
require('./unit/migrate/migration-list-resolver');
require('./unit/seed/seeder');
// require('./unit/interface'); ToDo Uncomment after fixed

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jan 6, 2019

Author Collaborator

This one looks like it got broken quite a while ago, I'll open a separate issue for it

@@ -986,7 +986,7 @@ module.exports = function(knex) {
});
});

it('Event: does not duplicate listeners on a copy with user params', function() {
it('Event: preserves listeners on a copy with user params', function() {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jan 6, 2019

Author Collaborator

Probably users are going to be confused if listeners set on original knex suddenly disappear when we copy it with user params, so this is an improvement.

kibertoad added some commits Jan 6, 2019

There doesn't seem to be a clear way to fix listener behaviour in Nod…
…e 6, so let's just ignore it for the time being, especially considering that we are dropping support for Node 6 in April anyway.

@kibertoad kibertoad requested a review from elhigu Jan 6, 2019

@kibertoad kibertoad closed this Jan 6, 2019

@kibertoad kibertoad reopened this Jan 6, 2019

asyncStackTraces: true,
});
const knex = Knex(
Object.assign({}, sqliteConfig, { asyncStackTraces: true })

This comment has been minimized.

Copy link
@elhigu

elhigu Jan 10, 2019

Collaborator

Was there a problem with original implementation?

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jan 11, 2019

Author Collaborator

Yes, Node 6 doesn't support spread, and we don't transpile tests. It used to work because it was never run in CI :)

expect(knex.client.listeners('query-response').length).to.equal(1);

const knexWithParams = knex.withUserParams();
knexWithParams.client.on('query', (obj) => {

This comment has been minimized.

Copy link
@elhigu

elhigu Jan 10, 2019

Collaborator

I'm starting to have second thoughts if this was a good idea to allow creating modifiable clones of knex instance which are sharing the same pool (feature came kind of accidentally). I think it is a really nice feature to have, but API could have separated cloning and setting same pools for them and setting user parameters of each instance more clearly... anyways test looks good 👍 Wanted to just mention how this small feature of user parameters has exploded a bit for allowing to set custom event handlers too, which is good.

});

it('adding listener to copy does not affect base knex', () => {
if (isNode6()) {

This comment has been minimized.

Copy link
@elhigu

elhigu Jan 10, 2019

Collaborator

I'm worried about this !node6 stuff. I would like to know the root cause why this could not work with it. Is babel doing something strange... maybe that polyfill thing is causing it?

});
});

it('copying does not result in duplicate listeners', () => {

This comment has been minimized.

Copy link
@elhigu

elhigu Jan 10, 2019

Collaborator

kind of duplicate with test "adding listener to copy does not affect base knex"

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jan 30, 2019

Author Collaborator

They are different, actually. One tests for what happens after you copy, another one what happens after you mutate the copy.

@elhigu
Copy link
Collaborator

elhigu left a comment

Changes seems good I suppose (I'm still gonna check it out one more time later on)... Only thing I found right now was the node6 exceptions, which should be investigated. My best guess is the polyfill causing it...

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

kibertoad commented Jan 11, 2019

@elhigu Worst case scenario, if we conclude that there is nothing that we can do about Node 6 behaviour in this case - we could start a 0.17.x branch early (April is coming up rapidly anyway) and drop support for Node 6 there already.

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

kibertoad commented Jan 30, 2019

@elhigu So what is currently remaining? I'll review the redundant test case; then you still want to test this thing further? We probably can't remove polyfills at this point.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jan 30, 2019

@elhigu So what is currently remaining? I'll review the redundant test case; then you still want to test this thing further? We probably can't remove polyfills at this point.

I was hoping to understand the reason, why that failing on node 6 happens to have some picture about how seriously it is broken and possibly to work around that breaking instead of ignoring the tests.

But I'm not completely against of merging this as is either, with some warnings in changelog about what might go wrong with node 6.

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

kibertoad commented Jan 30, 2019

@elhigu Let me do a quick pass over this pr then

@kibertoad

This comment has been minimized.

Copy link
Collaborator Author

kibertoad commented Jan 30, 2019

@elhigu Pushed the update to documentation. I don't think that investing too much time and energy into investigation makes much sense, April is coming soon anyway.
And this PR doesn't make Node 6 support any more broken than 0.16.0 already did (for all Node versions).

addressed comments

@elhigu elhigu merged commit 26868f8 into master Jan 31, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 85.199%
Details
@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Jan 31, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.