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

How do we support new dialects in the future? #2

Closed
sdepold opened this issue Nov 19, 2021 · 9 comments
Closed

How do we support new dialects in the future? #2

sdepold opened this issue Nov 19, 2021 · 9 comments

Comments

@sdepold
Copy link
Member

sdepold commented Nov 19, 2021

Questions:

  • Which requirements do we have towards the code, CI/CD, long-term support?
  • Should we keep the code in the core repo or somehow split the dialects / SQL queries from the core repo?
@sdepold sdepold created this issue from a note in Monthly syncs (Unprioritized Backlog) Nov 19, 2021
@sdepold sdepold moved this from Unprioritized Backlog to Prioritized (ordered by priority) in Monthly syncs Nov 19, 2021
@sdepold
Copy link
Member Author

sdepold commented Nov 19, 2021

Very related to #9.

@sdepold
Copy link
Member Author

sdepold commented Nov 19, 2021

Personal opinion. Happy to discuss and to try different approach:

I see the following problems right now:

  • The codebase has historically been growing with all the currently supported dialects. If we add more, it will make the npm package bigger for no real reason.
  • Builds are relatively slow since we always run for all dialects.
  • We merge the core "business logic" with SQL string generation.
  • It's rather hard to add more dialects because of overlaps.
  • Adding new dialects to the core implies future maintenance.

Possible solutions:

Option 1: Demand fully blown long-term support
When people add support for a new dialect, we enforce them to join the core team so that they can be pulled in if something goes south in the future. Furthermore, we demand a well configured and scalable CI/CD pipeline.

Option 2: Divide and conquer
We remove the dialect specific code chunks from the core and move them to their own dedicated bundle.

Something like

@sequelize/core --> Current Sequelize lib
@sequelize/dialect-mysql --> Everything that is related to mysql (or maria maybe)
@sequelize/dialect-snowflake --> Everything that is related to snowflake

This would make the core module smaller, we could iterated quicker and more focused on particular dialects and we could even bind the expected dialect's driver (@sequelize/dialect-postgres --> pg lib) to the package.
Additionally, people could create their own libs for dialects.

Crucial for this would be a proper testing strategy to ensure that all dialects, implement certain core behavior.

Current dialect related PRs:

@sdepold
Copy link
Member Author

sdepold commented Nov 19, 2021

I lean somehow towards option 2. This could go nicely into v7.

@WikiRik
Copy link
Member

WikiRik commented Nov 22, 2021

I agree with option 2. But I would like to see that we support most dialects within the sequelize organization so we're not dependent on individuals for maintaining the repos (although we have had a hand in not maintaining things sometimes as well).

I think this ties in with #6 as well, do we do both things at once or do we do the TypeScript conversion first and then move dialect-specific things to their own bundles?

@sdepold
Copy link
Member Author

sdepold commented Nov 23, 2021

Binit (IBM):
Structure for db2 (Unix compatible)/db2 for i (relies on IBM mainframe) is already split from the core. No overlaps except for maybe the test cases.

Mark Irish (IBM):
Internally there is a whitelist of supported dialects. Picking from it will make Sequelize lookup the right files in the internally dialects folder. Also, dialects expect the respective drivers (postgres dialect => pg package).
Function signatures are rather confusing due to the lack of typings. Will be much easier with TypeScript.
Core will become quite bloated if we keep going like that.

Rik: We also need to see what this means for the other repos like CLI, umzug and auto

Pedro:
Seconds the complexity of dialect specific functions. TypeScript migration was started but wasn't of big success. It won't be as simple as just taking out the folders but from what Binit says, it seems possible and dialects are self-contained. Anyway, this is definitely the right path.

Mark: There are tiny parts in the code which check if we currently use a particular dialect.

Jesse (Snowflake): It's mostly within the dialect files however. Also votes for approach 2. Current structure should allow for split quite okay. Could we maybe merge it anyway so that the community has access to it and we fix the structure then.

Sascha: I guess we will learn what refactoring is needed during the split.

Jesse: CI/CD will be tricky for any cloud providers. Unit test as much as we can.

Sascha: New dialects could be marked as experimental.

Binit: Support is only really complete if unit tests pass.

Sascha: Anyone against the idea of pulling out the dialects?

Pedro: Fears that the hybrid approach will leave it in a worse state when we don't manage to pull it out. However, all the work is already done so it should be factored in.

Next steps:

  • Update pull requests which add support for new dialects. Make sure everything is green
  • Merge them
  • Release of Sequelize with note about experimental support
  • Start refactoring dialect structure (@sdepold)

@sdepold sdepold moved this from Prioritized (ordered by priority) to Follow-up in Monthly syncs Nov 23, 2021
@sdepold
Copy link
Member Author

sdepold commented Jan 10, 2022

Q: What's the status of DB2 for i?

@sdepold
Copy link
Member Author

sdepold commented Jan 10, 2022

Mark:

Was trying of setting up a CI/CD system. You cannot spin up a IBM i environment. So tests would have to be executed against a real IBM i system. Those are right now all behind firewall. Management is not interested in opening a system for OS usage. Tried to run it against a server in Germany and a single test would take 8+ minutes.

Maybe we should just go with the "Snowflake way" where we only run the unit tests. Feature branch is quite out of sync with main. Mark is working on rebasing/merging main.

Mark/Sascha: Let's set up some smoke tests for db2 for i. Mark to prepare it and Sascha to add the credentials to our repo.

@sdepold
Copy link
Member Author

sdepold commented Jan 10, 2022

Rik: Zoe just prepared a huge linting PR that might cause conflicts. Let's merge this first and rebase db2 for i PR second.

@sdepold
Copy link
Member Author

sdepold commented Feb 17, 2022

Let's put all the currently supported dialects into a monorepo and maybe add other there as well in the future or let them have their own dedicated repo. CI/CD would target specific directories in the repo (to speed up the development process)

@sdepold sdepold closed this as completed Feb 17, 2022
Monthly syncs automation moved this from Follow-up to Done Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

2 participants