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.

Sign up for GitHub

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

Jump to bottom

feat(types): transition lib/errors #13710

Merged
merged 17 commits into from Dec 5, 2021
Merged

feat(types): transition lib/errors #13710

merged 17 commits into from Dec 5, 2021

Conversation

Keimeno
Copy link
Member

@Keimeno Keimeno commented Nov 24, 2021

Pull Request Checklist

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

  • Have you added new tests to prevent regressions?
  • Does npm run test or npm run 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

Transitions the lib/errors folder to TypeScript

@Keimeno Keimeno added the type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. label Nov 24, 2021
@Keimeno Keimeno self-assigned this Nov 24, 2021
@Keimeno Keimeno marked this pull request as draft November 24, 2021 22:51
Keimeno
Keimeno commented Nov 24, 2021
View reviewed changes
lib/errors/database-error.ts Outdated Show resolved Hide resolved
@Keimeno Keimeno force-pushed the feature/typescript-conversion-lib-errors branch from 284b87b to 2a1c9f6 Compare November 27, 2021 00:21
@Keimeno
Copy link
Member Author

Keimeno commented Nov 27, 2021

A few things I've noticed while migrating to typescript here. Most types are inconsistent with the JS code. Changes that were done here won't be breaking changes for JS users, but might be breaking changes for TS users.

Keimeno
Keimeno commented Nov 27, 2021
View reviewed changes
lib/errors/base-error.ts Outdated Show resolved Hide resolved
@Keimeno Keimeno force-pushed the feature/typescript-conversion-lib-errors branch from 2a1c9f6 to d3a3214 Compare November 27, 2021 00:30
@Keimeno Keimeno marked this pull request as ready for review November 27, 2021 01:15
@Keimeno Keimeno added the type: refactor For issues and PRs. Things that improve the code readability, maintainability, testability, etc. label Nov 27, 2021
sdepold
sdepold reviewed Nov 27, 2021
View reviewed changes
lib/dialects/mssql/async-queue.ts Show resolved Hide resolved
lib/errors/base-error.ts Outdated Show resolved Hide resolved
@Keimeno Keimeno added the status: in discussion For issues and PRs. Community and maintainers are discussing the applicability of the issue. label Nov 28, 2021
@Keimeno Keimeno removed the status: in discussion For issues and PRs. Community and maintainers are discussing the applicability of the issue. label Nov 28, 2021
sdepold
sdepold requested changes Dec 2, 2021
View reviewed changes
lib/errors/base-error.ts Outdated Show resolved Hide resolved
lib/dialects/mssql/async-queue.ts Show resolved Hide resolved
lib/errors/aggregate-error.ts Outdated Show resolved Hide resolved
lib/errors/bulk-record-error.ts Show resolved Hide resolved
test/integration/error.test.js Show resolved Hide resolved
lib/errors/validation-error.ts Show resolved Hide resolved
@sdepold
Copy link
Member

sdepold commented Dec 2, 2021

Some minor things really. Over all good stuff :)

wbourne0
wbourne0 reviewed Dec 2, 2021
View reviewed changes
Copy link
Member

@wbourne0 wbourne0 left a comment

Choose a reason for hiding this comment

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

Looks mostly fine, just some nits.

Can you remove the types from the comments? We should infer from TS types.

Also personal preference: can you explicitly specify public, protected, private, etc? IMO its better to do so since it forces you to think "should this be public?" (no strong opinions on this though)

lib/dialects/mssql/async-queue.ts Show resolved Hide resolved
lib/errors/base-error.ts Outdated Show resolved Hide resolved
lib/errors/optimistic-lock-error.ts Outdated Show resolved Hide resolved
lib/errors/optimistic-lock-error.ts Outdated Show resolved Hide resolved
lib/errors/optimistic-lock-error.ts Outdated Show resolved Hide resolved
lib/errors/validation-error.ts Show resolved Hide resolved
lib/errors/validation-error.ts Outdated Show resolved Hide resolved
lib/errors/validation-error.ts Outdated Show resolved Hide resolved
lib/errors/validation-error.ts Show resolved Hide resolved
@sdepold
Copy link
Member

sdepold commented Dec 3, 2021

Looks mostly fine, just some nits.

Can you remove the types from the comments? We should infer from TS types.

Also personal preference: can you explicitly specify public, protected, private, etc? IMO its better to do so since it forces you to think "should this be public?" (no strong opinions on this though)

I think the types in the comments are for the docs right? I was wondering the same however?

Regarding visibility: Let's have a poll on that :)

@sdepold
Copy link
Member

sdepold commented Dec 4, 2021

Seems we are leaning towards omitting public modifier and only go with private and protected.

@sdepold sdepold merged commit 8cdce6a into sequelize:main Dec 5, 2021
@Keimeno Keimeno deleted the feature/typescript-conversion-lib-errors branch December 5, 2021 17:49
@ephys ephys mentioned this pull request Dec 7, 2021
Merged
6 tasks
@github-actions
Copy link
Contributor

🎉 This PR is included in version 6.12.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Keimeno Keimeno mentioned this pull request Dec 15, 2021
Closed
@github-actions
Copy link
Contributor

🎉 This PR is included in version 6.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
@Keimeno @sdepold @aliatsis
feat(types): transition lib/errors (sequelize#13710)
b20aba6 
* feat(types): transition lib/errors

* feat(types): importing model type from existing types

* feat(types): don't export AsyncQueueError from lib/errors

* feat(types): storing key not value of enum in class

* feat(types): migrate async-queue to typescript

* fix(tests): adjust tests for typescript migration

* feat(types): using inline export for named exports

* feat(types): rework type definitions

* feat(types): export AsyncQueueError from errors/index

Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WikiRik WikiRik WikiRik left review comments

@wbourne0 wbourne0 wbourne0 left review comments

@sdepold sdepold sdepold approved these changes

Assignees

@Keimeno Keimeno

Labels
released on @v6-beta released type: refactor For issues and PRs. Things that improve the code readability, maintainability, testability, etc. type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Keimeno @sdepold @WikiRik @wbourne0