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

Closes sequelize/sequelize#9608. Added dialectModule parameter. #9609

Closed
wants to merge 2 commits into from
Closed

Closes sequelize/sequelize#9608. Added dialectModule parameter. #9609

wants to merge 2 commits into from

Conversation

catamphetamine
Copy link
Contributor

Added dialectModule parameter for Webpack.
Closes #9608

@codecov
Copy link

codecov bot commented Jun 28, 2018

Codecov Report

Merging #9609 into master will decrease coverage by 0.03%.
The diff coverage is 50%.

@sushantdhiman
Copy link
Contributor

Going to need some integration test for this, for all dialects

  1. Create a new test file dialect-module-configuration.test.js https://github.com/sequelize/sequelize/tree/master/test/integration/
  2. Test that both dialectModule and dialectModulePath works.
    1. dialectModule can be tested by first requiring module in your test, adding some dummy property to library module and then asserting sequelize.connectionManager.lib also got same property after initializing with custom module
    2. dialectModulePath can be tested simply by asserting if new Sequelize works without throwing MODULE NOT FOUND error

@catamphetamine
Copy link
Contributor Author

@sushantdhiman
I agree that tests are required for such a widely used library.
Added some stubs for tests for now.
Other people can possibly join too.

@sushantdhiman
Copy link
Contributor

Please let me know whenever you are willing to continue on this, I will reopen this.

Also there are actually very slim chances that someone will help you write those tests, if you need this feature I am afraid you will need to work on them yourself.

@yvele
Copy link

yvele commented Aug 29, 2018

@catamphetamine what can I do to help with this?

@catamphetamine
Copy link
Contributor Author

@yvele Don't ask me, ask the author of the library: he doesn't want to merge it in.

@sushantdhiman
Copy link
Contributor

@catamphetamine ah, blaming me for something I never did :)

As I said earlier #9609 (comment) I need proper tests before merging this. I can't merge something untested just like that

@yvele
Copy link

yvele commented Sep 5, 2018

How can I help? @catamphetamine you need to give me access to https://github.com/catamphetamine/sequelize I think 🤔

@sushantdhiman This issue is important to fix because I can't understand why sequelize is doing the require, it should be more simple and explicitly take the whole module (better for mocking, etc.)

@papb
Copy link
Member

papb commented Apr 4, 2021

Related: #13169

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