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(types): use retry-as-promised types for retry options to match documentation #15400

Merged
merged 8 commits into from Dec 8, 2022

Conversation

esetnik
Copy link
Contributor

@esetnik esetnik commented Dec 6, 2022

Pull Request Checklist

  • 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?
  • Does the name of your PR follow our conventions?

Description Of Change

I have corrected the types in the underlying retry-as-promised library so they can now be re-exported by sequelize to get the proper type for retry.

See DefinitelyTyped/DefinitelyTyped#63267 for details.

fzn0x
fzn0x previously approved these changes Dec 6, 2022
WikiRik
WikiRik previously approved these changes Dec 6, 2022
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few small differences between the current state and the types package but I do think we should prefer the types package over our own type. Thanks for this PR!

@esetnik
Copy link
Contributor Author

esetnik commented Dec 6, 2022

There are a few small differences between the current state and the types package but I do think we should prefer the types package over our own type. Thanks for this PR!

Yes there are. My hope is that this is not a breaking change for anyone. I'm happy to open a new PR against @types/retry-as-promised for any additional type changes that are necessary, but we would need to audit the underlying package to determine if those modifications are actually valid.

package.json Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@esetnik esetnik dismissed stale reviews from WikiRik and fzn0x via e7ae123 December 6, 2022 15:48
package.json Outdated Show resolved Hide resolved
ephys
ephys previously approved these changes Dec 6, 2022
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀🎉

WikiRik
WikiRik previously approved these changes Dec 6, 2022
@esetnik
Copy link
Contributor Author

esetnik commented Dec 6, 2022

Note that @mickhansen is going to refactor retry-as-promised to typescript so we can hold off on merging this PR. I will update this PR to depend on the new version of retry-as-promised with native types once it has been released.

@esetnik esetnik marked this pull request as draft December 6, 2022 16:17
@esetnik esetnik dismissed stale reviews from WikiRik and ephys via 44c043d December 7, 2022 21:19
@esetnik
Copy link
Contributor Author

esetnik commented Dec 7, 2022

Waiting on mickhansen/retry-as-promised#37

@esetnik esetnik marked this pull request as ready for review December 8, 2022 16:32
@esetnik esetnik requested review from WikiRik and ephys and removed request for WikiRik and ephys December 8, 2022 16:32
@esetnik esetnik requested review from fzn0x and WikiRik and removed request for fzn0x December 8, 2022 16:32
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If tests are green, we're good to go :)

@WikiRik WikiRik merged commit f2574ae into sequelize:main Dec 8, 2022
@esetnik
Copy link
Contributor Author

esetnik commented Dec 19, 2022

@ephys will this get a backport release to v6?

@WikiRik
Copy link
Member

WikiRik commented Dec 19, 2022

@esetnik we should be able to, can you open a new PR towards the v6 branch?

esetnik added a commit to esetnik/sequelize that referenced this pull request Dec 19, 2022
esetnik added a commit to esetnik/sequelize that referenced this pull request Dec 19, 2022
esetnik added a commit to esetnik/sequelize that referenced this pull request Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants