Replace instanceof Message usages (#729)

This replaces the usage of `instanceof Message` (and `Message` subclasses) with the new `isMessage` function. In addition, it adds some tests to verify behavior. `isMessage` can be used to determine whether a given object is any subtype of `Message` or is a specific `Message` by passing the type. It is recommended to use this instead of `instanceof Message`. **Example usage** ```typescript import { isMessage } from "@bufbuild/protobuf"; const user = new User({ firstName: "Homer", }); isMessage(user); // true isMessage(user, User); // true isMessage(user, OtherMessageType); // false ``` View the docs [here](./docs/runtime-api.md#identifying-messages). **Perf benchmarks:** ``` // isMessage large google.protobuf.FileDescriptorSet (1061190 bytes) x 60.52 ops/sec ±1.13% (63 runs sampled) tiny docs.User (4 bytes) x 3,386,238 ops/sec ±0.19% (100 runs sampled) scalar values (102 bytes) x 630,178 ops/sec ±0.19% (96 runs sampled) repeated scalar fields (195 bytes) x 401,749 ops/sec ±0.19% (98 runs sampled) map with scalar keys and values (186 bytes) x 225,122 ops/sec ±0.21% (100 runs sampled) repeated field with 1000 messages (2000 bytes) x 7,154 ops/sec ±0.42% (96 runs sampled) map field with 1000 messages (8890 bytes) x 3,594 ops/sec ±0.21% (98 runs sampled) ``` ``` // main large google.protobuf.FileDescriptorSet (1061190 bytes) x 60.74 ops/sec ±1.27% (64 runs sampled) tiny docs.User (4 bytes) x 3,416,634 ops/sec ±0.24% (97 runs sampled) scalar values (102 bytes) x 627,019 ops/sec ±0.23% (99 runs sampled) repeated scalar fields (195 bytes) x 397,073 ops/sec ±1.53% (97 runs sampled) map with scalar keys and values (186 bytes) x 224,147 ops/sec ±0.42% (98 runs sampled) repeated field with 1000 messages (2000 bytes) x 7,162 ops/sec ±0.39% (95 runs sampled) map field with 1000 messages (8890 bytes) x 3,307 ops/sec ±0.20% (97 runs sampled) ``` --------- Co-authored-by: Timo Stamm <[email protected]>
bufbuild · Mar 2, 2024 · 3be7c9d · 3be7c9d
1 parent 3864c00
commit 3be7c9d
Show file tree

Hide file tree

Showing 14 changed files with 137 additions and 39 deletions.
diff --git a/docs/migrating.md b/docs/migrating.md
@@ -37,7 +37,6 @@ to Protobuf-ES.
 |------------------------|-------------|---------------------|-------------|
 | Initializers           | ✅           | ❌                   | ✅           |
 | Plain properties       | ✅           | ❌                   | ✅           |
-| `instanceof`           | ✅           | ✅                   | ❌           |
 | JSON format            | ✅           | ❌                   | ✅           |
 | Binary format          | ✅           | ✅                   | ✅           |
 | TypeScript             | ✅           | ❌                   | ✅           |
@@ -423,12 +422,9 @@ declare var message: Example;
 
 ```diff
 - Example.is(message);
-+ message instanceof Example;
++ isMessage(message, Example);
 ```
 
-Note that `instanceof` has much better performance characteristics than `is()`.
-For that reason, we do not provide an equivalent to `isAssignable()`.
-
 
 ### Reflection
 

diff --git a/docs/runtime_api.md b/docs/runtime_api.md
@@ -13,6 +13,7 @@ provided by the library.
   - [Cloning messages](#cloning-messages)
   - [Comparing messages](#comparing-messages)
   - [Serializing messages](#serializing-messages)
+  - [Identifying messages](#identifying-messages)
 - [Enumerations](#enumerations)
 - [Extensions](#extensions)
   - [Extensions and JSON](#extensions-and-json)
@@ -314,6 +315,33 @@ JSON.
 To learn about serialization options and other details related to serialization, 
 see the section about [advanced serialization](#advanced-serialization).
 
+### Identifying messages
+
+To check whether a given object is a message, use the function [`isMessage`][src-isMessage].
+
+`isMessage` is _mostly_ equivalent to the `instanceof` operator. For
+example, `isMessage(foo, MyMessage)` is the same as `foo instanceof MyMessage`,
+and `isMessage(foo)` is the same as `foo instanceof Message`. 
+
+The advantage of `isMessage` is that it compares identity by the message type 
+name, not by class identity. This makes it robust against the dual package 
+hazard and similar situations, where the same message is duplicated.
+
+To determine if an object is any subtype of `Message`, pass that object to the
+function. To determine if an object is a specific type of `Message`, pass the 
+object as well as the type.
+
+```typescript
+import { isMessage } from "@bufbuild/protobuf";
+
+const user = new User({
+    firstName: "Homer",
+});
+
+isMessage(user);                    // true
+isMessage(user, User);              // true
+isMessage(user, OtherMessageType);  // false
+```
 
 ## Enumerations
 
@@ -1100,7 +1128,7 @@ function to your users that processes this message:
 ```ts
 export function sendUser(user: PartialMessage<User>) {
   // convert partial messages into their full representation if necessary
-  const u = user instanceof User ? user : new User(user);
+  const u = isMessage(user, User) ? user : new User(user);
   // process further...
   const bytes = u.toBinary();
 }
@@ -1178,6 +1206,7 @@ Note that any message is assignable to `AnyMessage`.
 [src-PlainMessage]: https://github.com/bufbuild/protobuf-es/blob/9b8efb4f4eb8ff8ce9f56798e769914ee2069cd1/packages/protobuf/src/message.ts#L137
 [src-AnyMessage]: https://github.com/bufbuild/protobuf-es/blob/9b8efb4f4eb8ff8ce9f56798e769914ee2069cd1/packages/protobuf/src/message.ts#L25
 [src-toPlainMessage]: https://github.com/bufbuild/protobuf-es/blob/51573c39ff38a9b43b6f7c22ba6b5ba40fa3ec3a/packages/protobuf/src/to-plain-message.ts#L29
+[src-isMessage]: https://github.com/bufbuild/protobuf-es/blob/3864c00709c444d5cf2cef694345b9beea7b3ed9/packages/protobuf/src/is-message.ts#L31
 [@bufbuild/protobuf]: https://www.npmjs.com/package/@bufbuild/protobuf
 [@bufbuild/protoplugin]: https://www.npmjs.com/package/@bufbuild/protoplugin
 [pkg-protoplugin]: https://www.npmjs.com/package/@bufbuild/protoplugin

diff --git a/packages/protobuf-bench/README.md b/packages/protobuf-bench/README.md
@@ -10,5 +10,5 @@ server would usually do.
 
 | code generator      | bundle size             | minified               | compressed         |
 |---------------------|------------------------:|-----------------------:|-------------------:|
-| protobuf-es         | 96,999 b      | 41,438 b | 10,763 b |
+| protobuf-es         | 97,510 b      | 41,660 b | 10,845 b |
 | protobuf-javascript | 394,384 b  | 288,654 b | 45,122 b |
diff --git a/packages/protobuf-test/src/edition-feature-resolver.test.ts b/packages/protobuf-test/src/edition-feature-resolver.test.ts
@@ -33,6 +33,7 @@ import {
   FeatureSet_Utf8Validation,
   FeatureSetDefaults,
   FeatureSetDefaults_FeatureSetEditionDefault,
+  isMessage,
   protoInt64,
   ScalarType,
 } from "@bufbuild/protobuf";
@@ -633,7 +634,7 @@ describe("FeatureResolver", function () {
       ...descExtensions: DescExtension[]
     ): FeatureSet;
     function getDefaults(edition: Edition, ...rest: unknown[]) {
-      if (rest[0] instanceof FeatureSetDefaults) {
+      if (isMessage(rest[0], FeatureSetDefaults)) {
         const compiledFeatureSetDefaults = rest[0];
         const resolver = FeatureResolver.create(
           edition,

diff --git a/packages/protobuf-test/src/is-message.test.ts b/packages/protobuf-test/src/is-message.test.ts
@@ -0,0 +1,65 @@
+// Copyright 2021-2024 Buf Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+import { describe, expect, test } from "@jest/globals";
+import { User as TS_User } from "./gen/ts/extra/example_pb.js";
+import { User as JS_User } from "./gen/js/extra/example_pb.js";
+import { isMessage } from "@bufbuild/protobuf";
+
+describe("isMessage", () => {
+  test("subclass of Message", () => {
+    const user = new TS_User({
+      firstName: "Homer",
+      lastName: "Simpson",
+    });
+
+    expect(isMessage(user)).toBeTruthy();
+    expect(isMessage(user, TS_User)).toBeTruthy();
+  });
+  test("returns false if expected Message property is not a function", () => {
+    const user = new TS_User({
+      firstName: "Homer",
+      lastName: "Simpson",
+    });
+    // @ts-expect-error: Setting to a boolean to force a failure
+    user.toJson = false;
+
+    expect(isMessage(user, TS_User)).toBeFalsy();
+  });
+  test("null returns false", () => {
+    expect(isMessage(null)).toBeFalsy();
+    expect(isMessage(null, TS_User)).toBeFalsy();
+  });
+  test("non-object returns false", () => {
+    expect(isMessage("test")).toBeFalsy();
+    expect(isMessage("test", TS_User)).toBeFalsy();
+  });
+  test("mixed instances", () => {
+    const user = new TS_User({
+      firstName: "Homer",
+      lastName: "Simpson",
+    });
+
+    expect(isMessage(user, JS_User)).toBeTruthy();
+  });
+  test("type guard works as expected", () => {
+    const user: unknown = new TS_User();
+    if (isMessage(user)) {
+      expect(user.toJsonString()).toBeDefined();
+    }
+    if (isMessage(user, TS_User)) {
+      expect(user.firstName).toBeDefined();
+    }
+  });
+});
diff --git a/packages/protobuf-test/src/mixing-instances.test.ts b/packages/protobuf-test/src/mixing-instances.test.ts
@@ -13,10 +13,7 @@
 // limitations under the License.
 
 import { describe, expect, test } from "@jest/globals";
-import {
-  MessageFieldMessage as TS_MessageFieldMessage,
-  MessageFieldMessage_TestMessage as TS_MessageFieldMessage_TestMessage,
-} from "./gen/ts/extra/msg-message_pb.js";
+import { MessageFieldMessage as TS_MessageFieldMessage } from "./gen/ts/extra/msg-message_pb.js";
 import { MessageFieldMessage_TestMessage as JS_MessageFieldMessage_TestMessage } from "./gen/js/extra/msg-message_pb.js";
 
 describe("mixing message instances", () => {
@@ -37,14 +34,12 @@ describe("mixing message instances", () => {
 });
 
 describe("mixing message instances in the constructor", () => {
-  test("normalizes by creating a new instance", () => {
+  test("matches expected message", () => {
     const test = new JS_MessageFieldMessage_TestMessage({ name: "foo" });
     const message = new TS_MessageFieldMessage({
       messageField: test,
     });
     expect(message.messageField?.name).toBe("foo");
-    expect(message.messageField).toBeInstanceOf(
-      TS_MessageFieldMessage_TestMessage,
-    );
+    expect(message.messageField).toBe(test);
   });
 });
diff --git a/packages/protobuf/src/create-descriptor-set.ts b/packages/protobuf/src/create-descriptor-set.ts
@@ -55,6 +55,7 @@ import type { BinaryReadOptions, BinaryWriteOptions } from "./binary-format.js";
 import type { FeatureResolverFn } from "./private/feature-set.js";
 import { createFeatureResolver } from "./private/feature-set.js";
 import { LongType, ScalarType } from "./scalar.js";
+import { isMessage } from "./is-message";
 
 /**
  * Create a DescriptorSet, a convenient interface for working with a set of
@@ -76,12 +77,11 @@ export function createDescriptorSet(
     extensions: new Map<string, DescExtension>(),
     mapEntries: new Map<string, DescMessage>(),
   };
-  const fileDescriptors =
-    input instanceof FileDescriptorSet
-      ? input.file
-      : input instanceof Uint8Array
-        ? FileDescriptorSet.fromBinary(input).file
-        : input;
+  const fileDescriptors = isMessage(input, FileDescriptorSet)
+    ? input.file
+    : input instanceof Uint8Array
+      ? FileDescriptorSet.fromBinary(input).file
+      : input;
   const resolverByEdition = new Map<Edition, FeatureResolverFn>();
   for (const proto of fileDescriptors) {
     const edition =

diff --git a/packages/protobuf/src/create-registry-from-desc.ts b/packages/protobuf/src/create-registry-from-desc.ts
@@ -66,6 +66,7 @@ import type {
 import { createDescriptorSet } from "./create-descriptor-set.js";
 import type { Extension } from "./extension.js";
 import type { ExtensionFieldSource } from "./private/extensions.js";
+import { isMessage } from "./is-message";
 
 // well-known message types with specialized JSON representation
 const wkMessages = [
@@ -111,7 +112,7 @@ export function createRegistryFromDescriptors(
   IExtensionRegistry &
   IServiceTypeRegistry {
   const set: DescriptorSet =
-    input instanceof Uint8Array || input instanceof FileDescriptorSet
+    input instanceof Uint8Array || isMessage(input, FileDescriptorSet)
       ? createDescriptorSet(input)
       : input;
   const enums = new Map<string, EnumType>();

diff --git a/packages/protobuf/src/is-message.ts b/packages/protobuf/src/is-message.ts
@@ -17,16 +17,22 @@ import type { AnyMessage } from "./message.js";
 import { Message } from "./message.js";
 
 /**
- * Check whether the given object is an instance of the given message type.
- *
- * This function is equivalent to the `instanceof` operator. For example,
- * `isMessage(foo, MyMessage)` is the same as `foo instanceof MyMessage`, and
- * `isMessage(foo)` is the same as `foo instanceof Message`.
+ * Check whether the given object is any subtype of Message or is a specific
+ * Message by passing the type.
  *
  * Just like `instanceof`, `isMessage` narrows the type. The advantage of
  * `isMessage` is that it compares identity by the message type name, not by
  * class identity. This makes it robust against the dual package hazard and
  * similar situations, where the same message is duplicated.
+ *
+ * This function is _mostly_ equivalent to the `instanceof` operator. For
+ * example, `isMessage(foo, MyMessage)` is the same as `foo instanceof MyMessage`,
+ * and `isMessage(foo)` is the same as `foo instanceof Message`. In most cases,
+ * `isMessage` should be preferred over `instanceof`.
+ *
+ * However, due to the fact that `isMessage` does not use class identity, there
+ * are subtle differences between this function and `instanceof`. Notably,
+ * calling `isMessage` on an explicit type of Message will return false.
  */
 export function isMessage<T extends Message<T> = AnyMessage>(
   arg: unknown,
@@ -46,7 +52,7 @@ export function isMessage<T extends Message<T> = AnyMessage>(
   const actualType = (arg as { getType(): unknown }).getType();
   if (
     actualType === null ||
-    typeof actualType != "object" ||
+    typeof actualType != "function" ||
     !("typeName" in actualType) ||
     typeof actualType.typeName != "string"
   ) {

diff --git a/packages/protobuf/src/private/binary-format.ts b/packages/protobuf/src/private/binary-format.ts
@@ -27,6 +27,7 @@ import { assert } from "./assert.js";
 import { isFieldSet } from "./reflect.js";
 import type { ScalarValue } from "../scalar.js";
 import { LongType, ScalarType } from "../scalar.js";
+import { isMessage } from "../is-message";
 
 /* eslint-disable prefer-const,no-case-declarations,@typescript-eslint/no-explicit-any,@typescript-eslint/no-unsafe-argument,@typescript-eslint/no-unsafe-assignment,@typescript-eslint/no-unsafe-member-access,@typescript-eslint/no-unsafe-call,@typescript-eslint/no-unsafe-return */
 
@@ -210,7 +211,7 @@ function readField(
           readMessageField(reader, new messageType(), options, field),
         );
       } else {
-        if (target[localName] instanceof Message) {
+        if (isMessage(target[localName])) {
           readMessageField(reader, target[localName], options, field);
         } else {
           target[localName] = readMessageField(

diff --git a/packages/protobuf/src/private/field-wrapper.ts b/packages/protobuf/src/private/field-wrapper.ts
@@ -16,6 +16,7 @@ import { Message } from "../message.js";
 import type { MessageType } from "../message-type.js";
 import type { DescExtension, DescField } from "../descriptor-set.js";
 import { ScalarType } from "../scalar.js";
+import { isMessage } from "../is-message";
 
 /* eslint-disable @typescript-eslint/no-explicit-any -- unknown fields are represented with any */
 
@@ -40,7 +41,7 @@ export function wrapField<T extends Message<T>>(
   type: MessageType<T>,
   value: any,
 ): T {
-  if (value instanceof Message || !type.fieldWrapper) {
+  if (isMessage(value) || !type.fieldWrapper) {
     return value as T;
   }
   return type.fieldWrapper.wrapField(value);

diff --git a/packages/protobuf/src/private/json-format.ts b/packages/protobuf/src/private/json-format.ts
@@ -45,6 +45,7 @@ import { scalarZeroValue } from "./scalars.js";
 import { isScalarZeroValue } from "./scalars.js";
 import type { ScalarValue } from "../scalar.js";
 import { LongType, ScalarType } from "../scalar.js";
+import { isMessage } from "../is-message";
 
 /* eslint-disable no-case-declarations,@typescript-eslint/no-unsafe-argument,@typescript-eslint/no-unsafe-assignment,@typescript-eslint/no-unsafe-member-access,@typescript-eslint/no-unsafe-call */
 
@@ -385,7 +386,7 @@ function readField(
           return;
         }
         let currentValue = target[localName] as Message | undefined;
-        if (currentValue instanceof Message) {
+        if (isMessage(currentValue)) {
           currentValue.fromJson(jsonValue, options);
         } else {
           target[localName] = currentValue = messageType.fromJson(

diff --git a/packages/protobuf/src/private/util-common.ts b/packages/protobuf/src/private/util-common.ts
@@ -19,6 +19,7 @@ import type { MessageType } from "../message-type.js";
 import type { Util } from "./util.js";
 import { scalarEquals } from "./scalars.js";
 import { ScalarType } from "../scalar.js";
+import { isMessage } from "../is-message";
 
 /* eslint-disable @typescript-eslint/no-explicit-any,@typescript-eslint/no-unsafe-assignment,@typescript-eslint/no-unsafe-member-access,@typescript-eslint/no-unsafe-return,@typescript-eslint/no-unsafe-argument,no-case-declarations */
 
@@ -52,7 +53,7 @@ export function makeUtilCommon(): Omit<Util, "newFieldList" | "initFields"> {
             if (
               sourceField &&
               sourceField.kind == "message" &&
-              !(val instanceof sourceField.T)
+              !isMessage(val, sourceField.T)
             ) {
               val = new sourceField.T(val);
             } else if (
@@ -104,7 +105,7 @@ export function makeUtilCommon(): Omit<Util, "newFieldList" | "initFields"> {
             const mt = member.T;
             if (member.repeated) {
               t[localName] = (s[localName] as any[]).map((val) =>
-                val instanceof mt ? val : new mt(val),
+                isMessage(val, mt) ? val : new mt(val),
               );
             } else {
               const val = s[localName];
@@ -118,7 +119,7 @@ export function makeUtilCommon(): Omit<Util, "newFieldList" | "initFields"> {
                   t[localName] = val;
                 }
               } else {
-                t[localName] = val instanceof mt ? val : new mt(val);
+                t[localName] = isMessage(val, mt) ? val : new mt(val);
               }
             }
             break;
@@ -239,7 +240,7 @@ function cloneSingularField(value: any): any {
   if (value === undefined) {
     return value;
   }
-  if (value instanceof Message) {
+  if (isMessage(value)) {
     return value.clone();
   }
   if (value instanceof Uint8Array) {