Cleanup

[bufdev]

Just a tracking task from review.

Important:

• Conform to Go Style Guide :-) comments, splitting up of files, we should have a Golang eng do a pass with Timo
• DOCS - we should have extensive examples, justification about why this is better than alternatives
• Have someone who is not Timo who is also an expert in JS do a deep dive, probably Paul, optimally get some external feedback
• Update LICENSE to to 2021-2022 ✅

Random comments:

• Evaluate moving private/protoplugin to new repository protoplugin-es? Probably not to start but worth a conversation.
• Move cmd/internal to internal.
• See comments on Add documentation #35, tldr get rid of options if possible ✅
• Update go.mod to at least 1.17 (not really needed but) ✅
• Rename GOOG to GOOGLE_ in the Makfile :-) ✅

[paul-sachs]

Some notes on initial read through:

Build + Docs

• Readme should have some instructions to actually build anything. make build currently just fails saying bazel command doesn't exist. Maybe need some prerequisites listed somewhere?
• We seem to be using npx to run some commands (jest, tsc, etc). This defaults to using locally installed packages but will fallback to installing the newest version to global. IMO that seems incorrect, we should fail if the tool is not installed locally.

Codestyle and nits

packages/protobuf

• imports are using .js extensions, which work because typescript knows these files are the same, but it does look a little jarring. This is because we're relying on tsc alone to compile modules which doesn't have great defaults. Consider moving to something like vite may provide a better library output format. However, adding these tools will make it slower to compile so there are always tradeoffs.
• There seem to be a lot of circular dependencies (imports to ../../index.js) which although not immediately blocking, can present some tricky situations in the future (depending on static usage) so it's good to avoid.

Summary

All in all, the codebase looks very solid and the issues I've raised above are relatively minor.

[timostamm]

Thanks for taking a look, @paul-sachs!

Readme should have some instructions to actually build anything. make build currently just fails saying bazel command doesn't exist. Maybe need some prerequisites listed somewhere?

We have a CONTRIBUTING.md that also lists prerequisites. They're a bit involved unfortunately, because we have to build parts of the protocolbuffers repository.

Once we make this repository public, users will be pointed to to this file (and others) as per the GH docs.

We seem to be using npx

I like npx because of its ubiquity, and hate it because of the issues you mentioned. It appears to be pretty much the standard though. I've been using a straight call into ./node_modules/.bin before I knew it, but it npx traverses the tree, which is a bit more resilient in workspaces. If you think node_modules is worth it, I'm happy to change it. Storybook for example seems to use it quite liberally too, so 🤷

imports are using .js extensions

Yes, they are used because it's the only sensible way to run ECMAScript module based code via "type": "module" in node.js through tsc. That's the reason we use them in generated code as well. I'm aware of the fancier alternatives, but for code where I really want conformance, I haven't found an alternative to tsc. (They abandoned the specification some time ago, so with everybody else, you're always a ginea pig to some degree).

There seem to be a lot of circular dependencies (imports to ../../index.js)

The well-known types in @bufbuild/protobuf do that, because it would otherwise complicate the code generator quite a bit. Do you think there are concrete drawbacks? Do you know a bundler that would trip over it?

---

Aside from that, do you have some feedback on the library API and on the generated code? Those are the two areas we're committing to by publishing, and any issues we can catch now will be much easier to fix than later.

[paul-sachs]

I like npx because of its ubiquity, and hate it because of the issues you mentioned. It appears to be pretty much the standard though. I've been using a straight call into ./node_modules/.bin before I knew it, but it npx traverses the tree, which is a bit more resilient in workspaces. If you think node_modules is worth it, I'm happy to change it. Storybook for example seems to use it quite liberally too, so 🤷

My suggestion here would be instead to just use npm exec tsc -- <any args here> which respects node_modules directories without the problems of npx.

Do you think there are concrete drawbacks? Do you know a bundler that would trip over it?

Mostly I've just run into issues where if there are circular dependencies, some static references can complain about being undefined. Depending on the tool digesting it, it may cause problems. These kinds of things can probably be fixed and would probably benefit from a concrete example where it failed. It's mostly just a code smell to me at this point.

Aside from that, do you have some feedback on the library API and on the generated code?

No comments there yet, the code seems clean and fairly minimal. Exposing the static conversion methods (fromBinary, fromJson, etc) is super useful from a functional programming perspective. The inheritance chain is pretty shallow so that's good. I am digging more into implementation details today so I may have more thoughts on these within the next two days.

[timostamm]

We seem to be using npx

Looks like with npm v7, npx is just an alias for npm exec.

circular imports with wkt

Noted in #64 👍

[paul-sachs]

Looks like with npm v7, npx is just an alias for npm exec.

Wild, it can sometimes be really hard to track these things. It still seems weird to me to use it in the context of a build which should always have the proper dependency installed but this is good know.

[paul-sachs]

One thing I've noted that i've had slight hickup with:

// where User is imported from generated code
const [someState, setSomeState] = useState<User>();
...
setSomeState((prev) => ({
  ...prev,
  deactivated: true,
}));

The above will throw a TS error since the produced value does not have the expected shape (missing inherited methods). We can get around this with:

setSomeState((prev) => new User({
  ...prev,
  deactivated: true,
}));

But using class constructors like this within immutable structures like state is a little misleading.

With grpc_web code, I was able to use User.AsObject. I suppose I could use Partial<User> but that would also make other properties optional which is incorrect.

[paul-sachs]

I think I had spoken with @timostamm some time ago about this so maybe i'm just missing something to bridge the gap.

Edit:

Ah, found something. I could do:

import { User } from '....';
import { Message } from '@bufbuild/protobuf';

type UserJSON = Omit<User, keyof Message>;
...
const [someState, setSomeState] = useState<UserJSON>();
...
setSomeState((prev) => ({
  ...prev,
  deactivated: true,
}));

This works as expected. Perhaps just exposing that type alias can help with these immutable situations.

[timostamm]

It's PlainMessage<T>. Thanks for bringing this up, it should have more visible documentation.

[paul-sachs]

So here's a fun thing i've noticed and am thoroughly confused by:

import {
  User,
} from "../../../../../bufwebrequests/src/gen/buf/alpha/registry/v1alpha1/user_pb";
import { Timestamp } from "@bufbuild/protobuf";

const user = new User({
  id: "some-id",
  createTime: new Timestamp({
    nanos: 0,
    seconds: BigInt(0),
  }),
});

console.log(user.createTime.nanos ?? "Nothing");

Perhaps I'm doing something weird with Timestamp but this output "Nothing" all the time. Maybe unrelated to type generation though, but it's causing issues in my PR since we seem to be losing some information when updating users.

[paul-sachs]

So looking at the node_modules a little more closely, it looks like @bufbuild/protobuf/dist/esm/google/protobuf/timestamp_pb.js doesn't expose nanos or seconds as properties, which leads to some misleading types since the TS definitions say they do.

[timostamm]

So here's a fun thing i've noticed

It's a bug! 🎉 ✨

Just FYI: You could also use this:

const user = new User({
  id: "some-id",
  createTime: {
    nanos: 0,
    seconds: BigInt(0),
  },
});

But explicitly initializing with 0 is not necessary, because that's the default value. That means new Timestamp({nanos: 0}) is the same as new Timestamp(). Also BigInt(0) doesn't work if BigInt is unavailable, but protoInt64.parse(0) does. We should add this to the docs.

[paul-sachs]

Interesting. The actual use case I ran into this on was updating the state of a given user:

setSomeState((prev) => new User({
  ...prev,
  deactivated: true,
}));

After this operation, the nested createTime field was missing those properties and in fact failed instanceof checks as well. I assume the root cause is similar since the spread operator would have set the Timestamp instance as it was, similar to new Timestamp().

My case above was obviously just a toy case to reproduce the more complicated version.