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
better-sqlite3: update for version 7 #55144
better-sqlite3: update for version 7 #55144
Conversation
types/better-sqlite3/index.d.ts
Outdated
// TypeScript Version: 3.0 | ||
|
||
import Integer = require("integer"); | ||
// TypeScript Version: 4.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any risk to doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this impacts interoperability across types, why this bump? (complete removal is completely ok, but not using cutting edge, there should be reason why we use specific TS version)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got compilation errors (that I didn't investigate) in versions of TypeScript prior to 3.8. In 753d3b6, I lowered this from 4.3 to 3.8.
(Also, it has to be at least 3.2, because that's when BigInts were added to TypeScript.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also, it has to be at least 3.2, because that's when BigInts were added to TypeScript.)
that's OK, on DT 3.6 is the latest. I'd not bother with older versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll leave things as is, at 3.8. Let me know if that's wrong.
@EvanHahn Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PR
Code ReviewsBecause this PR edits the configuration file, it can be merged once it's reviewed by a DT maintainer. You can test the changes of this PR in the Playground. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 55144,
"author": "EvanHahn",
"headCommitOid": "753d3b6214f2591242cc3ac3f5e53e5902a0206a",
"lastPushDate": "2021-08-16T14:09:27.000Z",
"lastActivityDate": "2021-08-16T22:48:04.000Z",
"mergeOfferDate": "2021-08-16T20:03:16.000Z",
"mergeRequestDate": "2021-08-16T22:48:04.000Z",
"mergeRequestUser": "EvanHahn",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"popularityLevel": "Well-liked by everyone",
"pkgInfo": [
{
"name": "better-sqlite3",
"kind": "edit",
"files": [
{
"path": "types/better-sqlite3/better-sqlite3-tests.ts",
"kind": "test"
},
{
"path": "types/better-sqlite3/index.d.ts",
"kind": "definition"
},
{
"path": "types/better-sqlite3/tsconfig.json",
"kind": "package-meta",
"suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-tsconfigjson) and not moving towards it (check: `compilerOptions.lib.0`, `compilerOptions.target`)"
}
],
"owners": [
"Morfent",
"matrumz",
"sant123",
"loghorn",
"andykais",
"mrkstwrt"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "peterblazejewicz",
"date": "2021-08-16T20:02:41.000Z",
"isMaintainer": true
},
{
"type": "stale",
"reviewer": "sant123",
"date": "2021-08-13T20:50:50.000Z",
"abbrOid": "8a9b0dd"
}
],
"mainBotCommentID": 898711223,
"ciResult": "pass"
} |
2434d01
to
8a9b0dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@peterblazejewicz, @sant123 Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@EvanHahn: Everything looks good here. I am ready to merge this PR (at 753d3b6) on your behalf whenever you think it's ready. If you'd like that to happen, please post a comment saying:
and I'll merge this PR almost instantly. Thanks for helping out! ❤️ (@Morfent, @matrumz, @sant123, @Loghorn, @andykais, @mrkstwrt: you can do this too.) |
@sant123 Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
Ready to merge |
I just published |
This PR:
Integer
withBigInt
s (closes [@types/better-sqlite3] Types are out of date, Integer package no longer needed thanks to BigInts. #46381)db.checkpoint()
methodmemory
optionunsafeMode
methodnpm test better-sqlite3
.If changing an existing definition: