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

fix: not deep clone custom dialectModule option #16967

Open
wants to merge 3 commits into
base: v6
Choose a base branch
from

Conversation

JerrysShan
Copy link

Description Of Change

deep clone custom dialectModule option cause unexpected behavior.
If dialectModule is a complex object, I hope it is a reference rather than a deep clone.

@JerrysShan
Copy link
Author

@WikiRik May I ask when this pull request can be merged?

@WikiRik
Copy link
Member

WikiRik commented Jan 29, 2024

Can you share a bit more on why this is better. Do you have an example of where it causes unexpected behaviour as you mentioned?

This behaviour also seems to be present in the v7 alphas, where we converted the ConnectionManager to TypeScript. That should also be updated in the case that this is considered a beneficial change.

@JerrysShan
Copy link
Author

Can you share a bit more on why this is better. Do you have an example of where it causes unexpected behaviour as you mentioned?

This behaviour also seems to be present in the v7 alphas, where we converted the ConnectionManager to TypeScript. That should also be updated in the case that this is considered a beneficial change.

In the case I encountered, the dialectModule object was customized, and the object has a property _ctx , but the descriptor enumable of this property was false; this property was ignored during deep clone, resulting in unexpected behavior.

@JerrysShan
Copy link
Author

JerrysShan commented Jan 31, 2024

The other options of sequelize conifg are simple objects or values, and deep clone is expected to have no side effects; but the dialectModule option is special and needs to be an object containing the createConnection function.

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

Successfully merging this pull request may close these issues.

None yet

2 participants