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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lneves12
Copy link

@lneves12 lneves12 commented Jul 10, 2020

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 lneves12 changed the title Makes mysqljs easier to minify and support minifier mangle Makes mysqljs easier to minify and support minifier mangle - fix: Error: Received packet in the wrong sequence after minifying Jul 10, 2020
@lneves12
Copy link
Author

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

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
Copy link
Author

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

dougwilson commented Jul 16, 2020

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
Copy link
Author

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
Copy link
Author

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

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
Copy link
Author

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
Copy link
Author

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants