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

Updated README with info about multiple references #1607

Closed
wants to merge 2 commits into from

Conversation

mccbala
Copy link

@mccbala mccbala commented Aug 4, 2016

This issue occurs when multiple references are made to knex. It would be great if there is a way to avoid this or if this occurence is documented. I had to search for a lot of time and finally found this fix here http://stackoverflow.com/a/25854598/1459670

This issue occurs when multiple references are made to knex. It would be great if there is a way to avoid this or if this occurence is documented. I had to search for a lot of time and finally found this fix here http://stackoverflow.com/a/25854598/1459670
@mccbala mccbala closed this Aug 4, 2016
@elhigu
Copy link
Member

elhigu commented Aug 4, 2016

Either you forgot to document the first part of the trick, where in your main app first you monkey patch Knex.knex to initialized instance.

If it is really found from there already I'm pretty sure that thing you documented is just internal implementation detail, not a tested feature. Doing require('knex').knex sounds like anti-pattern which shouldn't be used.

Instead you can create your own module which creates singleton instance of knex with your configuration if you like to achieve functionality above.

I would just create it once in main application and then pass it to everywhere else that it is used.

@mccbala mccbala deleted the patch-2 branch August 4, 2016 10:29
@mccbala mccbala restored the patch-2 branch August 4, 2016 10:33
@mccbala
Copy link
Author

mccbala commented Aug 4, 2016

@elhigu Yes. I forgot the monkey patch part. Now added that in the next commit.

@mccbala mccbala reopened this Aug 4, 2016
@elhigu
Copy link
Member

elhigu commented Aug 4, 2016

@rhys-vdw whats your opinion on this? Do we want to promote this kind of solution? Probably I'll go to give my second opinion to that stackoverflow article.

@mccbala
Copy link
Author

mccbala commented Aug 4, 2016

@elhigu This may not be the best approach but I required this solution as I was creating data models in ORM style in which I wanted to use knex calls directly instead of writing a singleton instance of knex.

@elhigu
Copy link
Member

elhigu commented Sep 5, 2017

I reviewed this again and I'm sure I don't want to promote this in docs. Closing.

@elhigu elhigu closed this Sep 5, 2017
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.

None yet

2 participants