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

Extract dialects out of the core #13722

Open
16 of 18 tasks
sdepold opened this issue Nov 29, 2021 · 13 comments
Open
16 of 18 tasks

Extract dialects out of the core #13722

sdepold opened this issue Nov 29, 2021 · 13 comments
Labels
breaking change For issues and PRs. Changes that break compatibility and require a major version increment. type: refactor For issues and PRs. Things that improve the code readability, maintainability, testability, etc.

Comments

@sdepold
Copy link
Member

sdepold commented Nov 29, 2021

In order to support additional dialects more easily in the future, we want to pull the dialect specific files out of the core and move them into dedicated npm modules.

Relates to sequelize/meetings#2

@Keimeno Keimeno added this to the v7 milestone Nov 29, 2021
@WikiRik WikiRik added breaking change For issues and PRs. Changes that break compatibility and require a major version increment. type: refactor For issues and PRs. Things that improve the code readability, maintainability, testability, etc. labels Nov 29, 2021
@WikiRik WikiRik removed this from the v7 milestone Oct 23, 2022
@ephys ephys pinned this issue Mar 20, 2024
@ephys
Copy link
Member

ephys commented Mar 20, 2024

We're finally working on this

The high level plan is to have individual packages for each dialect, such as @sequelize/postgres, @sequelize/mariadb, @sequelize/oracle, etc

Each package will provide all the dialect-specific code. This means that it will be much easier for the community to create third-party dialect packages, even though they would currently still need to use many unstable internal APIs. This second part is something we'll improve in the future by providing a clean public dialect API, such as @sequelize/core/dialect-toolbox

Each package will also include the relevant connector library (pg, mariadb, tedious, etc). Users won't have to figure out which version should be installed anymore.

Finally, the dialect-specific options will be better checked in both typing and runtime checks

When initializing Sequelize, users will now need to provide the dialect package instead of a dialect name. Here is what it would look like:

import { PostgresDialect } from '@sequelize/postgres';

new Sequelize({
  dialect: PostgresDialect,
  // dialect-specific options
  username: 'demo',
  password: 'demo',
  native: true,

  // sequelize options
  pool: { max: 5 },
});

In this version, all options are provided to Sequelize itself. Sequelize then picks the ones that are dialect specific and gives them to the dialect instance when initializing it.

The dialectOptions option would be removed. All dialect-specific options would be moved to the top-level option bag.

The dialect would expose a static method called getSupportedOptions that is used to:

  • Make sure all user-provided options are supported by either Sequelize or the dialect
  • Make sure no dialect-specific option conflicts with Sequelize

Because credentials are dialect-specific, we need to change the signature of the constructor to only accept an option bag. The old new Sequelize(database, username, password) one does not make sense for some dialects.

@ephys ephys unpinned this issue Mar 20, 2024
@jdgriffith
Copy link

I believe this is a great direction to take this. Will this apply to v6 or is this a v7 initiative?

@WikiRik
Copy link
Member

WikiRik commented Mar 21, 2024

This is a v7 initiative, and will also continue on in v8 most likely

@jdgriffith
Copy link

v7 I think* has been worked for the last two years if I'm not mistaken. Do we think v7 will reach a stable release in the foreseeable future?

@WikiRik
Copy link
Member

WikiRik commented Mar 22, 2024

We've discussed the release of v7 today with our core team. The v7 alphas are already quite stable. After the initial extraction of dialects we want to move into a beta phase for v7 while we work on updating CLI and help bigger third-party packages migrate to v7. During that beta phase we will be more careful about breaking changes that we add.
See more here; https://github.com/sequelize/meetings/wiki/Meeting-2024%E2%80%9003%E2%80%9022#latest-v7-release-plan

Doing this work on v6 is counterproductive and requires a lot more work for us.

@ephys
Copy link
Member

ephys commented Mar 28, 2024

@WikiRik for the task about updating the documentation, note that we now have this project to keep track of which PRs still need to be documented: https://github.com/orgs/sequelize/projects/5

@ephys
Copy link
Member

ephys commented Mar 28, 2024

If we're going to add support for better-sqlite3, maybe we should rename the @sequelize/sqlite to @sequelize/sqlite3 to reflect the name of the library that is used by it. We'd then be able to add @sequelize/better-sqlite3

@WikiRik
Copy link
Member

WikiRik commented Mar 28, 2024

If we're going to add support for better-sqlite3, maybe we should rename the @sequelize/sqlite to @sequelize/sqlite3 to reflect the name of the library that is used by it. We'd then be able to add @sequelize/better-sqlite3

Bit more verbose, but I was also thinking about @sequelize/sqlite/better-sqlite possibly. Users shouldn't need to care which adapter they use, but if they have a preference they can use another one we provide. Alternatively both @sequelize/sqlite and @sequelize/better-sqlite would also be fine with me. I like @sequelize/sqlite less because it forces us towards an implementation and not just the dialect name

@ephys
Copy link
Member

ephys commented Mar 28, 2024

The problem with @sequelize/sqlite/better-sqlite is that it requires installing both dialect libraries. If we want to deduplicate code between the two, we can just publish @sequelize/abstract-sqlite that is not connector-dependant

Users shouldn't need to care which adapter they use

If we start proposing multiple adapters, they should make an informed decision on which one they want to use. Otherwise, we'd just pick the one we consider to be the best. Naming one /sqlite indicates that it's the one users should use by default, when instead we should document the differences

@WikiRik
Copy link
Member

WikiRik commented Mar 28, 2024

If we start proposing multiple adapters, they should make an informed decision on which one they want to use. Otherwise, we'd just pick the one we consider to be the best. Naming one /sqlite indicates that it's the one users should use by default, when instead we should document the differences

You've got a point there. Do we want to do this for the other dialects as well or is this only for sqlite since there is a relatively big demand to support better-sqlite?

@ephys
Copy link
Member

ephys commented Mar 28, 2024

To be consistent, we probably should. Considering the demand has only ever been for sqlite, that may not be worth the trouble.

@ephys
Copy link
Member

ephys commented Mar 28, 2024

Following the changes done in #17222, it will be very important to have a clear page that explains how to configure the credentials per dialect. Right now it's in the "dialect specific things" page, which is a bit too buried for my liking. The "how to connect to your database" chapter should probably go at the very start of the documentation, right after "getting started" and before "defining models". I think it would make sense to have one page per dialect that first gives the basics of connecting to that database type, and has sub-chapters for other dialect-specific options

The other dialect-specific things on that page should probably be moved to where they can be used too, so that page could be deleted

@ephys
Copy link
Member

ephys commented Apr 8, 2024

Considering DB2 is really DB2 for Linux, Unix, Windows, and ibmi is really DB2 for IBM i, should we rename the packages to @sequelize/db2-luw and @sequelize/db2-ibmi respectively?

Following discussion with @WikiRik, based on IBM's branding, they should be @sequelize/db2 and @sequelize/db2-ibmi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change For issues and PRs. Changes that break compatibility and require a major version increment. type: refactor For issues and PRs. Things that improve the code readability, maintainability, testability, etc.
Projects
None yet
Development

No branches or pull requests

5 participants