sidorares / node-mysql2

:zap: fast mysqljs/mysql compatible mysql driver for node.js
https://sidorares.github.io/node-mysql2/
MIT License
3.96k stars 595 forks source link

chore(*): move to let/const #849

Closed SimonSchick closed 5 years ago

SimonSchick commented 5 years ago

IMPORTANT: VIEW WITHOUT WHITESPACE CHANGES

This does a few things: 1) Convert all usage of var to let/const 2) Beef up eslint rules to ensure the proper implementation of the former 3) Enabled strict mode (has to do with iteration in for in loops) 4) Remove dead code 5) Fixed some broken code (see REVIEW comments). 6) Emit some warn events when also logging to console.

Please note that tests are failing right now, I noticed a dead loop inside of a test that I fixed, please see the build log for explanation.

SimonSchick commented 5 years ago

Please note that the diff is particularly huge due to husky auto applying prettier and changing indents, not sure why this hit me in particular.

Some files, especially the benchmark files, cause huge diffs.

SimonSchick commented 5 years ago

Please also note that I only enabled lint rules that I deemed necessary, primarily finding unused and undefined references and definition order.

SimonSchick commented 5 years ago

If this PR is too big I can break it down into multiple chunks (per folder).

sidorares commented 5 years ago

looks like there are some real test failures, not just lint or something. Have you tried investigating them @SimonSchick ? Do you need any help?

SimonSchick commented 5 years ago

@sidorares as described I added at least 1 test failure which is caused by a loop I 'fixed'. Tbh the test output is kinda hard to read (since all sorts of non failures as logged as well) but I'm getting pretty much all of the same failures as https://travis-ci.org/sidorares/node-mysql2/jobs/424179988 does.

sidorares commented 5 years ago

I'll try to read more carefully your comments later today or tomorrow. We need to fix code or tests before merging

SimonSchick commented 5 years ago

Fixed the test-auth-switch test failure, I accidently removed a marker read when removing unused variables, restored that now.

SimonSchick commented 5 years ago

ping

SimonSchick commented 5 years ago

@sidorares hi

SimonSchick commented 5 years ago

poke

sidorares commented 5 years ago

hi @SimonSchick sorry, terribly busy last few weeks

we need to fix failing tests / conflicts to be able to merge. CI is still failing

SimonSchick commented 5 years ago

I specifically asked you to look at the the failing test case as the test that is failing right now was previously not even running. I will resolve the conflicts once CI passes :)

SimonSchick commented 5 years ago

On another note, is there a reason you execute nvm ls-remote in travis? Seems to serve no purpose but slow down builds.

sidorares commented 5 years ago

On another note, is there a reason you execute nvm ls-remote in travis? Seems to serve no purpose but slow down builds.

we can remove that, was trying to debug node version issue long time ago

SimonSchick commented 5 years ago

Rebased and removed the redundant version printing.

SimonSchick commented 5 years ago

Hey @sidorares had any time to look at that failing test? Otherwise I will comment out the failing test.

SimonSchick commented 5 years ago

Rebased again, can you please look into this?

sidorares commented 5 years ago

hi @SimonSchick I fixed the test but did something that caused my repo in a state GH can't create pull request for continuation of your commit. Could you cherry-pick 18bafedd5aac88aa06214da8478c2736b98daf16 and 54ef9dc7a595d4e7b38fe48b224137c75da41491 from my remote into your branch and we'll use this PR?

SimonSchick commented 5 years ago

done

sidorares commented 5 years ago

looks my issues were related to GH outage today. I'll try to re-trigger CI now

sidorares commented 5 years ago

looks my issues were related to GH outage today. I'll try to re-trigger CI now

SimonSchick commented 5 years ago

done

SimonSchick commented 5 years ago

Sorry for the comment spam, the outage was/is pretty bad lol.

SimonSchick commented 5 years ago

I added your changed but reworded the commits (force push), doesn't seem like they are showing up correctly tho.

SimonSchick commented 5 years ago

Hey @sidorares you might have to re-trigger these builds manually :)

SimonSchick commented 5 years ago

It's working! 😄

sidorares commented 5 years ago

Huh, finally it's there. Thanks for your work @SimonSchick and sorry it took so long time, last month was super busy for me