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
Use NoticeMessage type for pg.Client 'notice' event. #51183
Conversation
@groner Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through. This is a live comment which I will keep updated. 1 package in this PR
Code ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. 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": 51183,
"author": "groner",
"headCommitOid": "8503fc50cc2863d15214056b4119fa2461327bc1",
"lastPushDate": "2021-02-11T18:07:09.000Z",
"lastActivityDate": "2021-02-15T18:36:55.000Z",
"maintainerBlessed": false,
"mergeOfferDate": "2021-02-14T01:02:10.000Z",
"mergeRequestDate": "2021-02-15T18:36:55.000Z",
"mergeRequestUser": "groner",
"hasMergeConflict": false,
"isFirstContribution": true,
"popularityLevel": "Popular",
"pkgInfo": [
{
"name": "pg",
"kind": "edit",
"files": [
{
"path": "types/pg/index.d.ts",
"kind": "definition"
},
{
"path": "types/pg/package.json",
"kind": "package-meta-ok"
},
{
"path": "types/pg/pg-tests.ts",
"kind": "test"
}
],
"owners": [
"pspeter3",
"HoldYourWaffle"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "HoldYourWaffle",
"date": "2021-02-14T01:01:33.000Z",
"isMaintainer": false
}
],
"ciResult": "pass"
} |
🔔 @pspeter3 @HoldYourWaffle — please review this PR in the next few days. Be sure to explicitly select |
@groner The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
(indecipherable mumbling only ci bot will hear)
921fd91
to
8503fc5
Compare
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? Comparison details 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
Ready to merge |
I just published |
This breaks typescript compilation when node_modules/pg-protocol/dist/messages.d.ts:86:21 - error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.
86 readonly name = MessageName.copyData;
~~~~~~~~~~~
node_modules/pg-protocol/dist/messages.d.ts:164:21 - error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.
164 readonly name = MessageName.notice; |
@ujwal-setlur Apologies for breaking your build. This is my first time encountering this issue. It would be nice if this can be fixed in pg-protocol, but if that can't happen in a timely manner then I suppose reverting this is the sensible thing to do. |
Thanks @groner. Let’s give it a couple or days before reverting. For now, I have disabled isolatedModules, but I would like to re-enable it. |
… 'notice' event. by @groner (indecipherable mumbling only ci bot will hear)
@groner, It looks like the upstream will not fix this. Can you please roll this back? Thanks. |
… 'notice' event. by @groner (indecipherable mumbling only ci bot will hear)
This PR updates the
pg.Client
'notice'
event to use uses theNoticeMessage
type frompg-protocol
.npm test <package to test>
.If changing an existing definition:
This type first appears in pg-protocol 1.2.0, which was released with pg 8.0.1. I don't think the additional pg-protocol dependency will cause any conflicts because previous releases used a different name (pg-packet-stream).