Replace instanceof Message usages

[smaye81]

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

import { isMessage } from "@bufbuild/protobuf";

const user = new User({
    firstName: "Homer",
});

isMessage(user);                    // true
isMessage(user, User);              // true
isMessage(user, OtherMessageType);  // false

View the full docs here.

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)

[smaye81]

@timostamm A few questions:

• I wasn't sure if we should change the docs in runtime_api.md or migrating.md. They make a few mentions of instanceof but figured I'd check to see if we wanted to even document/advertise the usage of isMessage there.
• There's now a failing test in packages/protobuf-test/src/mixing-instances.test.ts. This is because if we swap out instanceof here, it returns true now where instanceof returned false due to the mixed instances. As a result, the test now fails because a new, normalized instance is no longer created.
• Calling isMessage with an actual instance of Message returns false right now, which is most likely not what we want. However, I wasn't sure of the best way to determine the explicit type of Message in the isMessage function. We could maybe check that arg only contains the properties returned by Object.getOwnPropertyNames(Message.prototype)?

Let me know what you think.

[srikrsna-buf]

• Calling isMessage with an actual instance of Message returns false right now, which is most likely not what we want. However, I wasn't sure of the best way to determine the explicit type of Message in the isMessage function. We could maybe check that arg only contains the properties returned by Object.getOwnPropertyNames(Message.prototype)?

I think we can let it be as is, because new Message() is not usable. It expects a subclass in every one of the methods.

• There's now a failing test in packages/protobuf-test/src/mixing-instances.test.ts. This is because if we swap out instanceof here, it returns true now where instanceof returned false due to the mixed instances. As a result, the test now fails because a new, normalized instance is no longer created.

I think we should remove/switch the test to use isMessage. We should ask users to not use instanceOf and depend on isMessage moving forward.

[smaye81]

I think we can let it be as is, because new Message() is not usable. It expects a subclass in every one of the methods.

That's fair, but if we do that, we should change the messaging/comments around isMessage because:

const msg = new Message();
isMessage(msg); // false
msg instanceof Message; // true

So, we can't really say 'This function is equivalent to the instanceof operator' or suggest it's a drop-in replacement. I know that new Message() is not really usable, but Hyrum's Law, etc, etc.

I think we should remove/switch the test to use isMessage. We should ask users to not use instanceOf and depend on isMessage moving forward.

I think in this case we'd have to remove it entirely because right now it's testing

describe("mixing message instances in the constructor", () => {
  test("normalizes by creating a new instance", () => {
  ...

and now that isMessage returns true, this no longer normalizes by creating a new instance.

[srikrsna-buf]

I think in this case we'd have to remove it entirely because right now it's testing

But we can test that the JS generated one will match the TS generated one using isMessage which I think was the point of the

That's fair, but if we do that, we should change the messaging/comments around isMessage

Agreed. We should just say "use to check if this is any subtype of Message or a specific Message by passing the type"

[smaye81]

@srikrsna-buf updated the test and docs. We should probably still document isMessage somewhere, but not entirely sure of the best place. Let me know what you think so far.

[srikrsna-buf]

We should probably still document isMessage somewhere, but not entirely sure of the best place. Let me know what you think so far.

I think we can document it in the runtime api, is there any other place you want to put it?

[smaye81]

We should probably still document isMessage somewhere, but not entirely sure of the best place. Let me know what you think so far.

I think we can document it in the runtime api, is there any other place you want to put it?

Nope, that works for me. I updated the PR with the docs.

[timostamm]

Nice, LGTM!

Can you update the PR description with an example and a link to the docs?