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(postgres): support connectionTimeoutMillis dialectOption #14119

Merged
merged 3 commits into from Feb 11, 2024

Conversation

mmfKupl
Copy link

@mmfKupl mmfKupl commented Feb 15, 2022

Pull Request Checklist

Please make sure to review and check all of these items:

  • 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

Add support of "connectionTimeoutMillis" option for PostgreSQL dialect
https://node-postgres.com/api/client

@mmfKupl mmfKupl changed the base branch from main to v6 February 15, 2022 17:02
@sdepold
Copy link
Member

sdepold commented Feb 15, 2022

I think this should target main and we cherry pick it over. How confident are we that the camel case notation is correct where all the other props seem to prefer underscores.

To be investigated:

Do we have tests for that?
Do we document the supported options somewhere?
Is there actually any reason to not just forward all passes option to the driver in question? Why would sequelize do a sanity check in the first place?
Can we make use of the drivers typescript annotations to prevent people passing rubbish?

@sdepold
Copy link
Member

sdepold commented Feb 15, 2022

Thanks for your pr :)

@ephys
Copy link
Member

ephys commented Feb 16, 2022

I agree with the sanity check being redundant. I think dialectOptions should be passed to the dialect as-is

@sdepold
Copy link
Member

sdepold commented Feb 16, 2022

I agree with the sanity check being redundant. I think dialectOptions should be passed to the dialect as-is

@mmfKupl Interested in doing that change?

@mmfKupl
Copy link
Author

mmfKupl commented Feb 16, 2022

I agree with the sanity check being redundant. I think dialectOptions should be passed to the dialect as-is

@mmfKupl Interested in doing that change?

Yes. I will change my pr with this comment

@fzn0x fzn0x added the type: feature For issues and PRs. For new features. Never breaking changes. label Feb 16, 2022
@schmod
Copy link
Contributor

schmod commented Jul 11, 2022

think dialectOptions should be passed to the dialect as-is

This could be confusing, because we actually have at least one sequelize-specific parameter in dialectOptions: clientMinMessages. As long as node-postgres never adds a parameter of the same name, I suppose we could just pluck that off and pass the rest over? Or just hand the whole thing over with the extra parameter intact?

@WikiRik WikiRik added the v6 label Nov 6, 2022
@ephys ephys requested a review from a team March 24, 2023 11:02
@ephys
Copy link
Member

ephys commented Mar 24, 2023

clientMinMessages, odbcConnectionString authentication, debug, and prependSearchPath are the 5 dialect options I could find that are handled by Sequelize

I think we'll leave things as they are right now and we'll revisit what dialectOptions is meant to be in Sequelize 8 when we split the dialects to their own packages. It should allow us to strictly type dialectOptions based on the dialect.

@ephys ephys changed the title feat(postgresql): add connectionTimeoutMillis into dialectOptions feat(postgres): support connectionTimeoutMillis dialectOption Mar 24, 2023
@ephys ephys enabled auto-merge (squash) February 11, 2024 17:44
@ephys ephys merged commit e81200e into sequelize:v6 Feb 11, 2024
52 checks passed
Copy link
Contributor

🎉 This PR is included in version 6.37.0 🎉

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
Labels
released type: feature For issues and PRs. For new features. Never breaking changes. v6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants