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

[pg-protocol] patch out the const enum in messages.d.ts #2489

Closed

Conversation

forivall
Copy link
Contributor

Alternative fix to #2473

This is the diff it generates to the typedefs:

--- dist/messages.const-enum.d.ts	2021-03-11 15:16:01.000000000 -0800
+++ dist/messages.d.ts	2021-03-11 15:16:01.000000000 -0800
@@ -1,33 +1,32 @@
 /// <reference types="node" />
 export declare type Mode = 'text' | 'binary';
-export declare const enum MessageName {
-    parseComplete = "parseComplete",
-    bindComplete = "bindComplete",
-    closeComplete = "closeComplete",
-    noData = "noData",
-    portalSuspended = "portalSuspended",
-    replicationStart = "replicationStart",
-    emptyQuery = "emptyQuery",
-    copyDone = "copyDone",
-    copyData = "copyData",
-    rowDescription = "rowDescription",
-    parameterStatus = "parameterStatus",
-    backendKeyData = "backendKeyData",
-    notification = "notification",
-    readyForQuery = "readyForQuery",
-    commandComplete = "commandComplete",
-    dataRow = "dataRow",
-    copyInResponse = "copyInResponse",
-    copyOutResponse = "copyOutResponse",
-    authenticationOk = "authenticationOk",
-    authenticationMD5Password = "authenticationMD5Password",
-    authenticationCleartextPassword = "authenticationCleartextPassword",
-    authenticationSASL = "authenticationSASL",
-    authenticationSASLContinue = "authenticationSASLContinue",
-    authenticationSASLFinal = "authenticationSASLFinal",
-    error = "error",
-    notice = "notice"
-}
+export type MessageName =
+    | "parseComplete"
+    | "bindComplete"
+    | "closeComplete"
+    | "noData"
+    | "portalSuspended"
+    | "replicationStart"
+    | "emptyQuery"
+    | "copyDone"
+    | "copyData"
+    | "rowDescription"
+    | "parameterStatus"
+    | "backendKeyData"
+    | "notification"
+    | "readyForQuery"
+    | "commandComplete"
+    | "dataRow"
+    | "copyInResponse"
+    | "copyOutResponse"
+    | "authenticationOk"
+    | "authenticationMD5Password"
+    | "authenticationCleartextPassword"
+    | "authenticationSASL"
+    | "authenticationSASLContinue"
+    | "authenticationSASLFinal"
+    | "error"
+    | "notice";
 export interface BackendMessage {
     name: MessageName;
     length: number;
@@ -83,7 +82,7 @@
 export declare class CopyDataMessage {
     readonly length: number;
     readonly chunk: Buffer;
-    readonly name = MessageName.copyData;
+    readonly name = "copyData";
     constructor(length: number, chunk: Buffer);
 }
 export declare class CopyResponse {
@@ -161,7 +160,7 @@
     readonly length: number;
     readonly message: string | undefined;
     constructor(length: number, message: string | undefined);
-    readonly name = MessageName.notice;
+    readonly name = "notice";
     severity: string | undefined;
     code: string | undefined;
     detail: string | undefined;

@brianc
Copy link
Owner

brianc commented Mar 12, 2021

Hmm...I'm confused here is this an option if merging #2490 is not an option? Because I'm leaning towards #2490 right now (it looks good to me!)

@forivall
Copy link
Contributor Author

Either one would be fine. If you merge that one, close this one and we're good here!

@brianc
Copy link
Owner

brianc commented Mar 12, 2021

hmmm thinking about this a bit....there's this PR that fixes this by just not making it a const enum. Do you think the linked PR preserves backwards compatibility more? Changing from an enum to a string constant is a breaking change right? And then i'd have to do semver major for that which seems like a lot when we could just remove the const? Thanks for your help on this btw!

@forivall
Copy link
Contributor Author

A const enum doesn't actually produce any code (except with the preserveConstEnums option, which you don't use); the generated code is equivalent to using string literal. So changing it to string literals may break people's typescript builds, but it won't cause runtime issues.

Changing it to a non-const enum is fine, though, it changes the usage from literals to property access on the enum object. as you were mentioning hot path code, you might want to evaluate if that perf would be an issue.

@forivall
Copy link
Contributor Author

Closing as the other solutions seem preferable.

@forivall forivall closed this Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants