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

MySQL node #131

Merged
merged 3 commits into from
Dec 13, 2019
Merged

MySQL node #131

merged 3 commits into from
Dec 13, 2019

Conversation

ericHao22
Copy link
Contributor

add MySQL node Inspired by Postgres node

@CLAassistant
Copy link

CLAassistant commented Nov 25, 2019

CLA assistant check
All committers have signed the CLA.

@janober
Copy link
Member

janober commented Nov 26, 2019

@ericHao22 Thanks a lot for creating this integration! Really great! I will try to review and merge it the next days.
Just had a short look but was wondering how hard you think it would be to remove the "knex" dependency? The reason is that I want to try to avoid adding dependencies unless there is no way around it. The reason is that most dependencies have other dependencies which each can cause problems, security vulnerabilities and in general, would n8n explode very soon.
Knex is one of the packages which by itself has a lot of other dependencies and if I install it by itself it is already 13 MB large (to compare mysql2 is just 2.5 MB).

@ericHao22
Copy link
Contributor Author

ericHao22 commented Nov 28, 2019

@janober
Thank you for your reply. I think it would not do many work to remove "knex" dependency.
The reason why I choose knex is because I prefer to use query builder to setup SQL in a functional way. However, "mysql2" is not like "pg-promise" to provide helper to do that kind of job for us, and that is what "knex" devote on. Knex also do many work to be compatible with different driver, and that may be the reason to cause the problem you mentioned above. But I indeed can make SQL string by myself without it.
Therefore, do you think it is suitable to use "mysql2" with raw SQL string? Thank you.

@ulentini
Copy link

Hello guys, I would love to have MySQL available, @janober do you have a roadmap for this?
Thanks!

@Giumex
Copy link

Giumex commented Nov 29, 2019

Thank for the integration guys. I'm really looking forward to having it.

@janober
Copy link
Member

janober commented Nov 29, 2019

Very sorry for the delay. Still very crazy right now. Should however get better in two weeks.

@ericHao22 It would be really great if you could remove the knex dependency if it is like you described. Will then review and merge it asap.

@ericHao22
Copy link
Contributor Author

Sorry for coming back late. @janober I had finished removing knex dependency from this MySQL node. Could you help me review it again? Thank you.

@janober
Copy link
Member

janober commented Dec 12, 2019

Thanks a lot @ericHao22 for taking care of that! Will check it later today and merge.

@janober janober merged commit a78da10 into n8n-io:master Dec 13, 2019
@janober
Copy link
Member

janober commented Dec 13, 2019

Great work. Thanks a lot. Got merged.

Only fixed one lint issue and renamed the node. Will try to release a new version of n8n with it tomorrow.

@ericHao22
Copy link
Contributor Author

No problem. Look forward to seeing the release of new version.

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

Successfully merging this pull request may close these issues.

5 participants