Skip to content

fix(types): adds missing function signature to Model.prototype.previous() types#13042

Merged
papb merged 12 commits into
sequelize:mainfrom
danielschwartz:fix/model-previous-typedef
Mar 23, 2021
Merged

fix(types): adds missing function signature to Model.prototype.previous() types#13042
papb merged 12 commits into
sequelize:mainfrom
danielschwartz:fix/model-previous-typedef

Conversation

@danielschwartz
Copy link
Copy Markdown
Contributor

Pull Request check-list

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

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • [N/A] Have you added new tests to prevent regressions?
  • [N/A] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

In the type definition for previous() on a model instance, the only specified signature is for when you pass in an argument to get a specific properties previous value. However, Sequelize will let you call previous() with no arguments and get an object back that maps all changed properties to their previous values. This PR adds that additional function signature to the type definitions so you can use previous() that way as well.

@Keimeno
Copy link
Copy Markdown
Member

Keimeno commented Mar 14, 2021

The type Record<string, unknown> definitely is a good addition, but doesn't offer strict type safety. Wouldn't it make more sense to return the type TCreationAttributes? Or maybe even Partial<TCreationAttributes>, since its properties can be undefined.

@danielschwartz
Copy link
Copy Markdown
Contributor Author

@Keimeno good call, I will update the PR with Partial<TCreationAttributes> as the return type and then hopefully we should be good to go

shivamklr and others added 8 commits March 15, 2021 13:27
Co-authored-by: john gravois <jagravois@gmail.com>
…12811 sequelize#12655)

Co-authored-by: sliterok <korobatov2011@yandex.ru>
Co-authored-by: William Gurzoni <william_luizat@hotmail.com>
* test(mysql, mariadb): improve transaction tests

- Greatly improve test for `SELECT ... LOCK IN SHARE MODE`
- Greatly improve test for deadlock handling

* fix(mysql): release connection on deadlocks

This is a follow-up for a problem not covered by sequelize#12841.

* refactor(mariadb): `query.js` similar to mysql's

* Update comments with a reference to this PR
@papb
Copy link
Copy Markdown
Member

papb commented Mar 15, 2021

Hello! Thanks for the PR. I see a lot of formatting changes here though, such as newlines and changing single-quotes to double-quotes, maybe you have prettier enabled or something, can you undo those and focus your PR on the exact intended change please?

@papb papb added the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Mar 15, 2021
@danielschwartz
Copy link
Copy Markdown
Contributor Author

@Keimeno @papb sorry about the auto formatting stuff. My VSCode was set to formatOnSave. Those formatting changes are now backed out and all the checks are passing. Let me know if you need anything else from me to get this merged. Thanks! 🚀

@Keimeno
Copy link
Copy Markdown
Member

Keimeno commented Mar 15, 2021

@danielschwartz happened to me all the time as well 😃.

A hacky fix that I'm using:

  1. Download the tbremer.path-settings extension
  2. Add the following setting to your settings.json:
"pathSettings.rules": [
    {
      "path": "sequelize",
      "settings": {
        "editor.formatOnSave": false,
      }
    }
  ]

This automatically disables the formatOnSave rule if sequelize is in your path name (without requiring to create a .vscode folder per project).

@danielschwartz
Copy link
Copy Markdown
Contributor Author

@papb @Keimeno I saw a new release was cut today but this wasn’t merged in yet. I also see this PR still has an “awaiting response” status.

Is there anything else this PR needs to be merged? Would love to have this fix merged in so I can stop patching my local version. Thanks!

@Keimeno
Copy link
Copy Markdown
Member

Keimeno commented Mar 22, 2021

We need to tag @papb for that, this PR was probably overlooked.

It's good if you additionally add typing tests with expect-type, but that can also be done in a separate PR.

@papb papb removed the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Mar 22, 2021
@papb
Copy link
Copy Markdown
Member

papb commented Mar 22, 2021

Hello! I am sorry for the delay. Can you please fix the conflict and add a test?

For good examples on how to test typescript typings, check the following PRs: #11368 #11379 #11520. As @Keimeno said, you can also use expect-type if you need more complex assertions.

@papb papb added the status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action label Mar 22, 2021
@danielschwartz
Copy link
Copy Markdown
Contributor Author

@papb this should be good to go. I've fixed the merge conflict caused by the latest release and also added a test. Let me know if you need anything else to get this merged in.

Copy link
Copy Markdown
Member

@papb papb left a comment

Choose a reason for hiding this comment

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

Nice! Thanks!

@papb papb merged commit 5b16b32 into sequelize:main Mar 23, 2021
@github-actions
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 6.6.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

jctaveras pushed a commit to jctaveras/sequelize that referenced this pull request Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants