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 Buffer to WhereValue type #11499

Merged
merged 3 commits into from Oct 16, 2019

Conversation

fubar
Copy link
Contributor

@fubar fubar commented Oct 2, 2019

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?
  • 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

where clauses that contain Buffer values (querying binary fields) fail Typescript validation due to a missing type declaration. Example:

Foo.findOne({
  where: { bar: Buffer.alloc(0) },
});

This PR adds the missing Buffer type to the WhereValue type definition.

I ran npm run test-typings

@codecov
Copy link

codecov bot commented Oct 2, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11499   +/-   ##
=======================================
  Coverage   96.27%   96.27%           
=======================================
  Files          94       94           
  Lines        9185     9185           
=======================================
  Hits         8843     8843           
  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 bd59b87...cfba292. Read the comment docs.

@SimonSchick
Copy link
Contributor

Can you please add a test for this as well?

@sushantdhiman sushantdhiman added status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. labels Oct 4, 2019
@fubar
Copy link
Contributor Author

fubar commented Oct 13, 2019

@SimonSchick if you tell me how, more than happy to. npm run test-typings is producing no output, even when I explicitly throw an error in any of the files in types/test.

@papb
Copy link
Member

papb commented Oct 13, 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!

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 #11379 #11520

Let me know if you need further help!

@fubar
Copy link
Contributor Author

fubar commented Oct 13, 2019

@papb right of course, makes sense. I've added a test for all WhereValue literals.

I've noticed some linting issues in those typescript files. What do you think of covering them under the existing eslint setup? Happy to put it a PR for it.

@SimonSchick
Copy link
Contributor

I'd rather use TSLint for that until eslint is fully feature on-par with it, PR is welcome though :)

@fubar
Copy link
Contributor Author

fubar commented Oct 13, 2019

TSLint is reaching end of life on Jan 1st and will be officially replaced by ESLint (see palantir/tslint#4534). We've been using the typescript-eslint parser and plugin in our repos, which works pretty well and allows the same set of rules to be applied to JS and TS.

@SimonSchick
Copy link
Contributor

I haven't checked on their progress, but that sounds promising feel free to ahead with that.

@papb papb added status: awaiting maintainer and removed status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action labels Oct 13, 2019
@fubar fubar force-pushed the fix/add-buffer-to-where-type branch from 5d6b33d to cfba292 Compare October 14, 2019 02:00
@fubar
Copy link
Contributor Author

fubar commented Oct 15, 2019

@papb quick follow-up - all tests are passing now.

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

@sushantdhiman sushantdhiman merged commit c3c767e into sequelize:master Oct 16, 2019
@sushantdhiman
Copy link
Contributor

🎉 This PR is included in version 5.19.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

4 participants