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
Revert "Use NoticeMessage type for pg.Client 'notice' event." #51513
Conversation
@groner Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PRCode ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. Status
Once every item on this list is checked, I'll ask you for permission to merge and publish the changes. InactiveThis PR has been inactive for 13 days — please try to get reviewers! Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 51513,
"author": "groner",
"headCommitOid": "b478d10f85c4a38a44516fc98f04dc7783f74794",
"lastPushDate": "2021-03-05T02:33:23.000Z",
"lastActivityDate": "2021-03-18T18:19:01.000Z",
"maintainerBlessed": false,
"hasMergeConflict": false,
"isFirstContribution": false,
"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": [],
"ciResult": "pass"
} |
🔔 @pspeter3 @HoldYourWaffle — please review this PR in the next few days. Be sure to explicitly select |
👋 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. |
@groner Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day! |
0b006c6
to
b478d10
Compare
Re-ping @pspeter3, @HoldYourWaffle: This PR has been out for over a week, yet I haven't seen any reviews. Could someone please give it some attention? Thanks! |
FYI, this issue has been fixed upstream: brianc/node-postgres#2490 |
@ujwal-setlur thanks for the update. Hopefully a new pg release will follow soon. I'll close this for now. |
@groner I am not sure you should close this PR. The way it was fixed upstream may not be compatible with your previous PR. For example, |
Reverts #51183, which has caused problems for multiple users using
isolatedModules: true
, due to the const enum inpg-protocol/lib/messages
.Hopefully this can be revisited in the future, if
pg-protocol
can be changed to expose the message interfaces without breaking isolated module builds.#51183 (comment)
brianc/node-postgres#2473