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

fix(types): add missing transaction typescript types (v5) #11621

Merged
merged 2 commits into from Oct 29, 2019

Conversation

aecorredor
Copy link
Contributor

@aecorredor aecorredor commented Oct 29, 2019

Pull Request check-list

  • 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?
  • Have you added new tests to prevent regressions?
  • 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

As stated in the sequelize docs (https://sequelize.org/master/class/lib/transaction.js~Transaction.html#static-get-LOCK), when performing find operations, a lock property can be passed, and that can be either true or false or also a lock level obtained from the actual instance, i.e. t1.LOCK.UPDATE, which ends up being a string.

The TypeScript types were failing when trying to pass a boolean value to lock or when trying to access t1.LOCK... on a transaction instance.

This PR adds those missing types.

Link to existing issue: #11178 (comment)

Link to v6-beta PR: #11620

@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #11621 into v5 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##               v5   #11621   +/-   ##
=======================================
  Coverage   96.27%   96.27%           
=======================================
  Files          94       94           
  Lines        9187     9187           
=======================================
  Hits         8845     8845           
  Misses        342      342

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2083c9a...a1d0608. Read the comment docs.

Copy link

@swaibat swaibat left a comment

Choose a reason for hiding this comment

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

@aecorredor great job

@papb papb added status: awaiting maintainer type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. labels Oct 29, 2019
Copy link
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.

LGTM, same as #11620

@sushantdhiman sushantdhiman merged commit 6c781d6 into sequelize:v5 Oct 29, 2019
@sushantdhiman
Copy link
Contributor

🎉 This PR is included in version 5.21.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@danconnell
Copy link
Contributor

When I try to use 5.21.2, I'm seeing these errors on compilation:

node_modules/sequelize/types/lib/transaction.d.ts:33:14 - error TS1086: An accessor cannot be declared in an ambient context.

33   static get LOCK(): LOCK;
                ~~~~

node_modules/sequelize/types/lib/transaction.d.ts:40:7 - error TS1086: An accessor cannot be declared in an ambient context.

40   get LOCK(): LOCK;
         ~~~~

Is this expected?

@papb
Copy link
Member

papb commented Nov 1, 2019

Definitely not expected, this is surprising, I thought our tests would have picked this up. Are you using some specific version of typescript or something that to the best of your knowledge could be involved in this?

@danconnell
Copy link
Contributor

Ah, upgrading to typescript@3.6.4 (we were on 3.5.3 - which is only a few months old) fixed the issue.

Weird. Thanks for suggesting that.

@papb
Copy link
Member

papb commented Nov 1, 2019

😬 Awesome. To be honest I didn't expect it to be that simple...

@notoriousmango
Copy link

notoriousmango commented Nov 9, 2019

I am getting the error

node_modules/sequelize/types/lib/transaction.d.ts:33:14 - error TS1086: An accessor cannot be declared in an ambient context.

33   static get LOCK(): LOCK;
                ~~~~

node_modules/sequelize/types/lib/transaction.d.ts:40:7 - error TS1086: An accessor cannot be declared in an ambient context.

40   get LOCK(): LOCK;
         ~~~~

My packages

"dependencies": {
    "@types/bluebird": "^3.5.28",
    "@types/node": "^12.12.7",
    "@types/validator": "^10.11.3",
    "mysql2": "^2.0.0",
    "sequelize": "^5.21.2",
    "typescript": "^3.7.2"
  },

should i downgrade my typescript to typescript@3.6.4

@papb
Copy link
Member

papb commented Nov 9, 2019

@notoriousmango Please try downgrading and let us know if it works.

Note: this is just in order to help us figure it out, of course Sequelize should work with the latest TS. We shall figure it out.

@notoriousmango
Copy link

notoriousmango commented Nov 10, 2019

@papb Sorry, my global typescript was outdated.
It works fine when I updated the global typescript

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants