Makes mysqljs easier to minify and support minifier mangle - fix: Error: Received packet in the wrong sequence after minifying

[lneves12]

Relying on the function's name of the packets makes the code not minifiable, because they will just shorten up in the minification.

Solution:

I added a new string property "_id" to all the packets and sequences. This way we can make sure the packet/sequence ids will not be minified.

All the tests are passing. I also tested this in a real application using minification and everything was working fine.

related to: #1655

[lneves12]

Hey @dougwilson !

I know that this library is not that maintained anymore. I wonder how willing are you to accept this kind of contribution or even if you have the time for that :)

I will make sure I fix the failing unit test left soon (there is only one failing)

Thanks!

[dougwilson]

I know that this library is not that maintained anymore.

This is a bit offensive, especially since this is not stated anywhere, nor did you ask. It is certainly not something you should be leading with if you actually want someone who is maintaining it to accept a contribute for you :) You may want to reflect on how you approach open source projects in which folks work on during their free time. Opening with an offending statement is not a great way to start off a conversation around doing you a favor :)

I looked at your PR when you opened it and it was not passing CI. IDK what you wanted me to do besides point that you when you already pointed it out when you opened the PR... I was waiting until you fixed it, but now I'm not sure if I'm looking forward to your follow through on this after you've made such a statement.

[lneves12]

That's definitely not what I meant. I am very sorry if I offended you in any way! I am also doing this in my free time.

I try to contribute as much as possible to opensource and be as respectful as possible. It was definitely not my intention to start this conversation like this. I am very sorry about that.

I just wanted to make sure I was going in the right direction to solve this problem.

I will make sure all the tests are passing.

[dougwilson]

Yes, I didn't comment on it when I first saw it as I didn't have any comments to make; i.e. it seems in the right direction to me, just waiting on the updated commits.

[lneves12]

Thanks! The CI is now passing

Sorry once more, I never meant to be disrespectful or to criticize your work. I respect your work a lot!

Let me know if there is anything else I have to do in order to make this PR ready to merge

[lneves12]

I squashed all the commits and rebased on top of the most recent master.

Let me know if there is anything else I can do to improve this PR.

Thanks!

[dougwilson]

Hi @lneves12 sorry I forgot to follow up on one item (your comment reminded me): it looks like this is not backwards-compatible as in some modules that integrate with this library to extend it fail completely with these changes, breaking them. An example is https://www.npmjs.com/package/zongji for instance. I haven't looked yet if there is a way to make this a backwards-compatible change or I just need to integrate it into the 3.0 alpha. You have have some thoughts, though, so just wanted to add it as a note.

[lneves12]

Thanks for the update.

Ahh, I see that zongji library relies on the internals function.constructor.name :/

I will keep thinking of other ways of implementing this without breaking those cases :)

Off the top of my head, I can only think of somehow overwriting function.constructor.name in order to force it to have the full name even if the function is minified, but that sounds super sketchy and I am not sure the impact that could have.

Since it's something not that critical, might be better to just include this, as you suggested, in a major and assume breaking changes rather than implementing a hacky solution

[lneves12]

@dougwilson I updated the PR to make the changes backward compatible. IIUC, it was breaking zongji because they have their own sequences/packets. This way the main protocol.js supports both. I am not sure how to test zongji integrated with mysqljs, some insight on that would be welcome

Let me know what you think

[dvargas92495]

hey, any update on this PR? Just ran into this issue in production and was wondering if there's anything could do to help get it across the finish line