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

feat: Migrate ConnectionManager to TypeScript #14652

Merged
merged 19 commits into from Jun 19, 2022
Merged

Conversation

ephys
Copy link
Member

@ephys ephys commented Jun 16, 2022

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

This PR migrates the AbstractConnectionManager to TypeScript. As usual it isn't just a type addition but a small refactor to work better with TypeScript.

Notable Change 1: Pool Normalization

A first notable change is the unification of how the connection pool works with and without replication.
Before this PR, if read replication was disabled, connectionManager.pool was an instance of sequelize-pool. If read replication was enabled, connectionManager.pool was an object that mimicked sequelize-pool and used two pools internally (one for read, one for write).

After this PR, connectionManager.pool is always an instance of ReplicationPool (new class) which dispatches to its internal read and write pools. If read replication is disabled, the read pool is disabled and the write pool is used for reads too.

This change makes the type much easier to work with.

Notable Change 2: Sequelize option priority

Which parameter takes priority in the Sequelize constructor is inconsistent:

// The following resulted in 'sequelize.config.username' being equal to 'user1'

new Sequelize('db1', 'user1', 'pwd1', {
  username: 'user2',
});

// The following resulted in 'sequelize.config.username' being equal to 'user2'

new Sequelize('mysql://user1:pwd1@localhost:1234/dbname', {
  username: 'user2',
});

After this PR, both versions prioritize the option bag over the string parameters. Both result in user2 being used.

Notable Change 3: Fixing another MySQL schema bug

Moved to #14657. The code is still in this PR but that other PR should be merged first.


Closes #13660

Squash Commit Message Body

If this is merged by someone else than me, here is the message body to use for the squash commit (don't forget the empty line at the start of the body, per conventional commits conventions):


BREAKING CHANGE: The signature of `connectionManager.pool` when read replication is disabled is now the same as when read replication is enabled. If read replication is disabled, you can access the actual pool through `connectionManager.pool.write`. If read replication is enabled, you can use both `connectionManager.pool.read` and `connectionManager.pool.write`.

@WikiRik
Copy link
Member

WikiRik commented Jun 17, 2022

Good work on this already! Let's indeed move the MySQL schema bug to another PR

@ephys ephys marked this pull request as ready for review June 17, 2022 15:51
@ephys
Copy link
Member Author

ephys commented Jun 17, 2022

Moved to #14657

package.json Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
src/dialects/abstract/index.ts Show resolved Hide resolved
test/unit/connection-manager.test.js Show resolved Hide resolved
test/integration/pool.test.ts Show resolved Hide resolved
test/unit/configuration.test.ts Show resolved Hide resolved
test/integration/replication.test.ts Show resolved Hide resolved
@WikiRik
Copy link
Member

WikiRik commented Jun 19, 2022

@ephys I'll leave the merging up to you

@ephys ephys merged commit b382a32 into main Jun 19, 2022
@ephys ephys deleted the ephys/ts-connection-manager branch June 19, 2022 20:27
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2022

🎉 This PR is included in version 7.0.0-alpha.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

[Feature Request] Expose pool diagnostic information
2 participants