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
Merged

Fix event listener duplication #2982

merged 6 commits into from Jan 31, 2019

Conversation

@kibertoad
Copy link
Collaborator

@kibertoad 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
@@ -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
Copy link
Collaborator Author

@kibertoad kibertoad Jan 6, 2019

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() {
Copy link
Collaborator Author

@kibertoad kibertoad Jan 6, 2019

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 2 commits Jan 6, 2019
…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 })
Copy link
Member

@elhigu elhigu Jan 10, 2019

Was there a problem with original implementation?

Copy link
Collaborator Author

@kibertoad kibertoad Jan 11, 2019

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) => {
Copy link
Member

@elhigu elhigu Jan 10, 2019

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()) {
Copy link
Member

@elhigu elhigu Jan 10, 2019

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', () => {
Copy link
Member

@elhigu elhigu Jan 10, 2019

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

Copy link
Collaborator Author

@kibertoad kibertoad Jan 30, 2019

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

Copy link
Member

@elhigu 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
Copy link
Collaborator Author

@kibertoad 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
Copy link
Collaborator Author

@kibertoad 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
Copy link
Member

@elhigu 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
Copy link
Collaborator Author

@kibertoad kibertoad commented Jan 30, 2019

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

@kibertoad
Copy link
Collaborator Author

@kibertoad 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).

@kibertoad kibertoad dismissed elhigu’s stale review Jan 30, 2019

addressed comments

@elhigu elhigu merged commit 26868f8 into master Jan 31, 2019
3 checks passed
@elhigu
Copy link
Member

@elhigu elhigu commented Jan 31, 2019

Thanks!

@kibertoad kibertoad deleted the fix/duplicate-listeners branch Dec 27, 2020
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 issues

Successfully merging this pull request may close these issues.

2 participants