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 (v6-beta) #11620

Merged
merged 2 commits into from Oct 29, 2019

Conversation

aecorredor
Copy link
Contributor

@aecorredor aecorredor commented Oct 28, 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)

@aecorredor
Copy link
Contributor Author

@aecorredor aecorredor commented Oct 28, 2019

Question, if this ends up getting merged, would it be added to all versions? (v4, v5, and v6-beta) or only to the latest stable one (v5)?

@codecov
Copy link

@codecov codecov bot commented Oct 28, 2019

Codecov Report

Merging #11620 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11620   +/-   ##
=======================================
  Coverage   96.26%   96.26%           
=======================================
  Files          94       94           
  Lines        9190     9190           
=======================================
  Hits         8847     8847           
  Misses        343      343

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 43c4f6b...93a27cb. Read the comment docs.

@papb
Copy link
Member

@papb papb commented Oct 28, 2019

Hello! I see you are a first-time contributor, thank you for taking the time to help Sequelize! I hope to see more PRs from you in the future!

I will now review your PR :)

@papb papb added the type: typescript label Oct 28, 2019
UPDATE: LOCK.UPDATE;
SHARE: LOCK.SHARE;
KEY_SHARE: LOCK.KEY_SHARE;
NO_KEY_UPDATE: LOCK.NO_KEY_UPDATE;
Copy link
Member

@papb papb Oct 28, 2019

Choose a reason for hiding this comment

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

Hello, I have a question, I don't use TypeScript much, why is both an enum and an interface necessary? Also why is only the enum being exported?

Copy link
Member

@papb papb Oct 28, 2019

Choose a reason for hiding this comment

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

Also why did you move the enum outside the export namespace Transaction block?

Copy link
Contributor Author

@aecorredor aecorredor Oct 28, 2019

Choose a reason for hiding this comment

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

So, if sequelize was entirely written in TS (and not only types were provided), we could just use an enum directly in the code and that would get compiled to an actual object (This is actually discouraged by some people and they advise to do this if you want to support runtime enums: https://medium.com/@martin_hotell/10-typescript-pro-tips-patterns-with-or-without-react-5799488d6680, look at point 3).

But, since here we are just providing types and not touching the code, we can't just do that. Here, an enum is just a TS type that basically says "look, whatever you say has this type, can only be one of these values". In our case, LOCK is an enum with 4 possible values which we're saying are strings. Now, we might first think that the static get LOCK and the get LOCK would return a type of LOCK (which is an enum). But that would not be correct by looking at the code. The actual code returns an object with four properties, UPDATE, SHARE, KEY_SHARE and NO_KEY_UPDATE. So, for our types to be correct, we need to tell static get LOCK and get LOCK that they return an object with those four properties, therefore the need for the interface LOCK, which models exactly that.

We only export the enum because we use it as one of the possible types for lock in the models file. The interface is just used in this file to indicate what the getter functions return.

I moved the enum outside the namespace because then it would conflict with the type declaration of the actual class getters also called LOCK.

Copy link
Member

@papb papb left a comment

Please add at least one test to make sure your implementation works as intended and to prevent regressions in the future.

Testing the TypeScript typings consists simply on checking if some code can be compiled. In other words, a typings test is just some TS code that should compile. What it does when executed is irrelevant, the test is simply to compile it without errors, as can be seen in our CI configuration (using npm run test-typings) here and here. Since it's just a compilation, getting no output from it means that it passed. Getting a compilation error is a failure. Note that if you directly throw an error from your typings test code, the test will still pass because throwing an error is something that compiles just fine.

For good examples on how to write typescript typings, check the following PRs: #11368 #11378 #11520

Let me know if you need further help!

@papb papb added the status: awaiting response label Oct 28, 2019
@aecorredor
Copy link
Contributor Author

@aecorredor aecorredor commented Oct 28, 2019

@papb added tests to use the lock property in all possible ways in different transactions.

@aecorredor aecorredor requested a review from papb Oct 28, 2019
papb
papb approved these changes Oct 29, 2019
Copy link
Member

@papb papb left a comment

Nice work, thanks!

@papb
Copy link
Member

@papb papb commented Oct 29, 2019

Question, if this ends up getting merged, would it be added to all versions? (v4, v5, and v6-beta) or only to the latest stable one (v5)?

No. This depends on what branch you opened the PR into. In this case:

image

Since this PR is for master, then it will be added only to v6-beta.

I think it's a good idea to have this change on v5 as well, so if you could open another PR with the same changes but targeting the v5 branch, that would be great.

About v4, note that we didn't provide typescript typings by then, so this fix does not apply to v4. Typings for v4 exist but are unofficial and kept in the DefinitelyTyped package called @types/sequelize.

@aecorredor
Copy link
Contributor Author

@aecorredor aecorredor commented Oct 29, 2019

Question, if this ends up getting merged, would it be added to all versions? (v4, v5, and v6-beta) or only to the latest stable one (v5)?

No. This depends on what branch you opened the PR into. In this case:

image

Since this PR is for master, then it will be added only to v6-beta.

I think it's a good idea to have this change on v5 as well, so if you could open another PR with the same changes but targeting the v5 branch, that would be great.

About v4, note that we didn't provide typescript typings by then, so this fix does not apply to v4. Typings for v4 exist but are unofficial and kept in the DefinitelyTyped package called @types/sequelize.

Ah, nice. I'll open the PR for v5 now. Thanks for all the info/help.

@aecorredor aecorredor changed the title fix(types): add missing transaction typescript types fix(types): add missing transaction typescript types (v6-beta) Oct 29, 2019
@sushantdhiman sushantdhiman merged commit 757a986 into sequelize:master Oct 29, 2019
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting response type: typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants