feat(options): Add M option

[pcj]

Fixes #596

In conjuction with https://github.com/stephenh/ts-poet/pull/28/files

[stephenh]

I don't have any better solutions to "making the npm registry of proto files" in terms of having the tooling know how to map the proto world --> each language binding world, so 🤷 this seems good to me, and if it follows the proto-gen-go approach, then all the better for following precedence.

Just out of curiosity, I saw that Buf has this new Connect/go/etc. plugin; are they solving the "map proto imports --> language imports" in a sufficiently different/interesting way?

[pcj]

Just out of curiosity, I saw that Buf has this new Connect/go/etc. plugin; are they solving the "map proto imports --> language imports" in a sufficiently different/interesting way?

I saw the connect announcement on HN, but TBH I know very little about it. From https://github.com/bufbuild/connect-go/blob/main/cmd/protoc-gen-connect-go/main.go, it looks like they are calling protoc-gen-go as a library to produce foo.pb.go, and using protoc-gen-go's subplugin architecture to generate foo.connect.pb.go. I don't see any interpretation of options in that part of the code, so it looks like they are not doing anything different.

[paralin]

I'm working on an RPC implementation which supports client -> server streams (which connect-go does not, nor does grpc-web or any other library I'm aware of) via using libp2p-mplex over WebSocket.

Using protowrap to call the protoc generator: https://github.com/aperturerobotics/starpc/blob/main/Makefile#L101

This PR (or one similar to it) would fix an issue I have with the import paths being generated:

import { OtherMessage } from '../../../../github.com/aperturerobotics/protobuf-project/example/other/other'

... need to remap this so that the project root - "../../../../github.com/aperturerobotics/protobuf-project" - is instead mapped to something like "../" (path to the repo root).

[pcj]

@paralin to clarify, are you saying that this feature would cause an issue for you? Or perhaps it's close to what you need, but not quite?

[paralin]

No! Actually I'm trying to support the case for an option to remap the imports. I think this would fix it (I'll test it out too)

Edit: sorry I've noticed I had a typo in the original post

[pcj]

@paralin thanks for the clarification. Note that the import path is constructed by ts-poet by the maybeRelativePath function, copied here:

export function maybeRelativePath(outputPath: string, importPath: string): string {
  if (!importPath.startsWith('./')) {
    return importPath;
  }
  // Drop the `./` prefix from the outputPath if it exists.
  const basePath = outputPath.replace(/^.\//, '');
  // Ideally we'd use a path library to do all this.
  const numberOfDirs = basePath.split('').filter((l) => l === '/').length;
  if (numberOfDirs === 0) {
    return importPath;
  }
  // Make an array of `..` to get our importPath to the root directory.
  const a: string[] = new Array(numberOfDirs);
  const prefix = a.fill('..', 0, numberOfDirs).join('/');
  return prefix + importPath.substring(1);
}

So in your example import { OtherMessage } from '../../../../github.com/aperturerobotics/protobuf-project/example/other/other', a debugger at return prefix + importPath.substring(1); would show prefix = ../../../.. and importPath = ./github.com/aperturerobotics/protobuf-project/example/other/other.

The prefix is derived from the outputPath (the directory where the protos are being generated). This PR will have no effect on that value. If you want it to look like import { OtherMessage } from '../github.com/aperturerobotics/protobuf-project/example/other/other', a different (but similar) feature would be required (or maybe play around with --ts_proto_out).

[paralin]

@pcj Thanks for looking into this. I checked the function you suggested & I think I've found a fix. Moved to stephenh/ts-poet#30

[pcj]

@stephenh anything else todo before merging this?

[stephenh]

@pcj would you mind creating a new integration/... test/example with a parameter.txt that has the -M opt passed in? That will be the best way to make sure this feature doesn't regress and continues working end-to-end.

Disclaimer we've got quite a few of those tests at this point. :-)

Separately, I had a chance to glance just a bit more at this; I'm still good with the current approach, of using ts-poet as a slice point to rewrite imports; I'm wondering if maybe another approach would have been to do it around here:

export function createTypeMap(request: CodeGeneratorRequest, options: Options): TypeMap {
  const typeMap: TypeMap = new Map();
  for (const file of request.protoFile) {
    // We assume a file.name of google/protobuf/wrappers.proto --> a module path of google/protobuf/wrapper.ts
    const moduleName = file.name.replace('.proto', '');

Where we'd use something like file.name or file.package to look up in options to know "what import path should we use for this".

And then maybe the . shouldn't be in this method:

export function impProto(options: Options, module: string, type: string): Import {
  const importString = `${type}@./${module}${options.fileSuffix}`;
  if (options.onlyTypes) {
    return imp('t:' + importString);
  } else {
    return imp(importString);
  }
}

As module would already have either ./ baked in (unless it was overridden), or have been overridden when creating the typeMap.

This might be tomato/tomato, but I'm wondering if "catching" the path sooner / before createTypeMap + impProto guess the wrong value (i.e. making the assumption the path will ./... based), and we have to later fix with a ts-poet mapping, would help the feature just work for what @paralin is looking to do?

As a disclaimer, I've not personally had to solve the "translating proto import X" --> "language import Y" (across separate protoc invocations/artifacts), so am really deferring to you guys for use cases/the best approach. Thanks!

[pcj]

@stephenh thanks, I'll take a closer look. The protoc-gen-go M option is actually like that, where you'd say M=google/protobuf/empty.proto=github.com/foo/bar/example. So the import filename is the map key, like you are suggesting.

Not sure if there are other side-effects, but that sounds promising.

[paralin]

I've posted a full example of what I'm trying to accomplish with the import remaps (leveraging the Go vendor/ tree) under stephenh/ts-poet#30

I don't think it's exactly the same issue as what's being addressed here - probably totally separate - although the fixes to use path.relative for maybeRelativePath might be useful.

[stephenh]

@paralin I believe this PR would hopefully help you replace these lines:

+      const thisProject = 'github.com/aperturerobotics/starpc';
+      if (ourModulePath.startsWith(thisProject)) {
+        // throw new Error(ourModulePath + ' : ' + modulePath);
+        const projectModulePath = path.dirname(ourModulePath.substring(thisProject.length))
+        ourModulePath = "./" + projectModulePath
+      }
+      if (modulePath.startsWith('./')) {
+        modulePath = modulePath.substr(2);
+        modulePath = './vendor/' + modulePath;

aperturerobotics/starpc@0dd17b1#diff-3d00f910ffe653a0e4cd63e3bbd9c320ab19cce074bffaa3f373d2f283f74e03R25

With a few -M parameters.

Although looking at your thisProject vs. vendor logic, I wonder if my suggestion to use this slice point:

export function createTypeMap(request: CodeGeneratorRequest, options: Options): TypeMap {
  const typeMap: TypeMap = new Map();
  for (const file of request.protoFile) {
    // We assume a file.name of google/protobuf/wrappers.proto --> a module path of google/protobuf/wrapper.ts
    const moduleName = file.name.replace('.proto', '');

Should do a similar is check of "is request.protoFile in the current compilation unit?". Right now ts-proto assumes it always is, but I'm pretty sure there is a way to tell which request.protoFiles are there are actual codegen output, and others that are included only as referenced/imported types...

I suppose how @pcj 's import map avoids needing to do this, b/c we assume any entry the user put in the -M import map, they're explicitly telling us is in a separate compilation unit. That seems fine/makes sense.

[paralin]

@stephenh The goal of that code is to leverage the Go vendoring system, which will automatically materialize any imports - i.e. "github.com/aperturerobotics/protobuf-project/example" - into the vendor/ tree with dependency version control, including the .proto and the .ts files there.

The Makefile symlinks the project to its fully qualified import path (declared in go.mod) to vendor/. So vendor/github.com/my/project goes to ./. Ts-proto only sees the fully qualified import paths.

So, I passed the prefix - "github.com/my/project" - as $PROJECT in the environment. Any files which have that prefix are considered to be in the same project, and use relative import paths - "./example"

Any paths which are /not/ in that prefix are considered to be located at ./vendor/, so ts-poet is correctly generating the relative imports - "../../vendor/github.com/..."

It seems that the portion of this which checks the module path for a given prefix, and treats those files as being in the project root ./, could be added as a generic option to ts-proto.

[pcj]

Haven't forgot about this but unable to get the docker integration tests to work on my arm mac. Switching to intel.

[paralin]

@pcj I've rebased your commits on "main"

@stephenh LGTM! :)

[stephenh]

@paralin awesome; thanks for rebasing. Two questions:

a) I think @pcj was going to try my suggestion of moving the slice point to createTypeMap, because then the -M option could probably be -M<proto package name>=<output path>, which matches the go -M option. Is that something you could take a look at?

We can still merge this as-is, and keep relying on ts-poets importMapping capability, but it seems like at some point it would be nice for -M to use proto package names, but then would be a breaking change if/when we switch to that later.

b) Could you add an integration test that shows the -M param used in parameters.txt and that it materially affects the output?

[paralin]

@stephenh How would that work if two protobuf files have the same package name but are from different locations in the tree?

I guess at least for my situation, updating the import mapping based on the relative path to the .proto file would be fine,

But I agree that if the option is -M it should take the same parameters as the -M for the Go protoc

[stephenh]

How would that work if two protobuf files have the same package name
but are from different locations in the tree?

...how does protoc-go handle it? 🤷 :-)

But I agree that if the option is -M it should take the same parameters
as the -M for the Go protoc

Yeah, I have the same intuition, but am really deferring to whatever you and @pcj are cool with actually doing the work for. :-)

I'm happy to merge this as-is, or hold off until either you or @pcj update it to use the protoc-go style -M. Wdyt?

[iamricard]

👋🏼 i'm interested in seeing this land - is there anything i can do help push this through? it seems like this solves something we want to achieve: saying "path/to/foo.proto" should become import foo from "@myorg/path/to/foo.ts".

[stephenh]

Ah sure, @iamricard , that'd be great; I think where the PR is at is:

a) try the approach in this comment:

#597 (comment)

Which would basically be a new PR, that moves the slice point from getTsPoetMaps, more into ts-proto itself, in the createTypeMap method, which would allow the keys to be more about "proto package --> JS path" than "original JS path --> actual JS path".

b) Include an integration/ test showing the new -M option working

And that's about it, I think.

[iamricard]

sounds good! i'll put something together, and thanks for the guidance 🙂

[iamricard]

👋🏼 my first attempt at this #672

[stephenh]

This should be fixed in #672 ; thanks @iamricard !