search

stephenh / ts-poet

A code generator DSL for typescript
Apache License 2.0
103 stars 13 forks source link

Using the Go vendor/ tree for automatic cross-project protobuf imports #30

Closed paralin closed 2 years ago

paralin commented 2 years ago

From #597

Example: https://github.com/aperturerobotics/starpc

Issue: generates an incorrect import path:

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

Looking at the call stack:

Seems that it counts the number of slashes & generates ../../../.. prefix plus the import path (github.com...) when it might be better to do ./ + the relative path (./other) instead.

paralin commented 2 years ago

There's one other thing to fix: for example:

import "github.com/aperturerobotics/project/project.proto";

message OtherMessage {
  .project.EchoMsg msg = 1;
}

Generates:

import { Hash } from '../../../../project'

Which /would/ be valid if this was in the vendor/ tree, but it's not - so it would be necessary to replace the importPath for some paths with certain prefixes (that match the root project tree).

In ts-poet/build/Import there is if (modulePath in importMappings) - I guess in this case, there'd also need to be if (ourModulePath in ourImportMappings) -> ./

paralin commented 2 years ago

@stephenh Okay I've hacked up a fix for both of these issues:

diff --git a/node_modules/ts-poet/build/Import.js b/node_modules/ts-poet/build/Import.js
index 955a7eb..0ad3a67 100644
--- a/node_modules/ts-poet/build/Import.js
+++ b/node_modules/ts-poet/build/Import.js
@@ -6,6 +6,7 @@ Object.defineProperty(exports, "__esModule", { value: true });
 exports.sameModule = exports.maybeRelativePath = exports.emitImports = exports.SideEffect = exports.Augmented = exports.ImportsAll = exports.ImportsDefault = exports.ImportsName = exports.Imported = exports.Implicit = exports.Import = exports.importType = void 0;
 const lodash_1 = __importDefault(require("lodash"));
 const path_1 = __importDefault(require("path"));
+const path = require("path");
 const Node_1 = require("./Node");
 const typeImportMarker = '(?:t:)?';
 const fileNamePattern = '(?:[a-zA-Z0-9._-]+)';
@@ -274,6 +275,15 @@ function emitImports(imports, ourModulePath, importMappings) {
         return '';
     }
     let result = '';
+    const thisProject = process.env.PROJECT;
+    let ourModuleImportPath = path.normalize(ourModulePath);
+    // HACK: if this is an import from our project, set the path accordingly
+    // github.com/aperturerobotics/protobuf-project/example/example -> ./example/example
+    if (thisProject && ourModuleImportPath.startsWith(thisProject)) {
+        ourModuleImportPath = './' + ourModuleImportPath.substr(thisProject.length + 1);
+    }
+    // result += `// ourModulePath: ${ourModulePath}\n`;
+    // result += `// ourModuleImportPath: ${ourModuleImportPath}\n`;
     const augmentImports = lodash_1.default.groupBy(filterInstances(imports, Augmented), (a) => a.augmented);
     // Group the imports by source module they're imported from
     const importsByModule = lodash_1.default.groupBy(imports.filter((it) => it.source !== undefined &&
@@ -288,7 +298,16 @@ function emitImports(imports, ourModulePath, importMappings) {
         if (modulePath in importMappings) {
             modulePath = importMappings[modulePath];
         }
-        const importPath = maybeRelativePath(ourModulePath, modulePath);
+        // HACK: if this is an import from a different project, use vendor/ tree.
+        if (thisProject && modulePath.startsWith('./')) {
+            if (!modulePath.substr(2).startsWith(thisProject)) {
+                modulePath = './vendor/' + path.normalize(modulePath);
+            } else {
+                modulePath = './' + modulePath.substr(3 + thisProject.length);
+            }
+        }
+        // result += `// modulePath: ${modulePath}\n`;
+        const importPath = maybeRelativePath(ourModuleImportPath, modulePath);
         // Output star imports individually
         unique(filterInstances(imports, ImportsAll).map((i) => i.symbol)).forEach((symbol) => {
             result += `import * as ${symbol} from '${importPath}';\n`;
@@ -337,17 +356,15 @@ function maybeRelativePath(outputPath, importPath) {
     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;
+    importPath = path.normalize(importPath);
+    outputPath = path.normalize(outputPath);
+    const outputPathDir = path.dirname(outputPath);
+    let relativePath = path.relative(outputPathDir, importPath);
+    if (!relativePath.startsWith('.')) {
+      // ensure the js compiler recognizes this is a relative path.
+      relativePath = './' + relativePath;
     }
-    // Make an array of `..` to get our importPath to the root directory.
-    const a = new Array(numberOfDirs);
-    const prefix = a.fill('..', 0, numberOfDirs).join('/');
-    return prefix + importPath.substring(1);
+    return relativePath;
 }
 exports.maybeRelativePath = maybeRelativePath;
 /** Checks if `path1 === path2` despite minor path differences like `./foo` and `foo`. */

Changes:

This creates a quite good way of working with Go projects:

syntax = "proto3";
package other;

import "github.com/aperturerobotics/starpc/echo/echo.proto";

message OtherMessage {
  // EchoMsg is the example echo message.
  .echo.EchoMsg echo_msg = 1;
}

Generates:

import { EchoMsg } from '../../vendor/github.com/aperturerobotics/starpc/echo/echo'

... and importing a path from the same project:

import "github.com/aperturerobotics/protobuf-project/example/other/other.proto";

... generates a relative import:

import { OtherMessage } from './other/other'

The full example is here:

Currently using the hacky patch under patches/

So: to make this actually mergeable into ts-poet:

stephenh commented 2 years ago

Fixed in #31 (afaiu :-))

paralin commented 2 years ago

@stephenh Not exactly - that only fixed half of the issue.

stephenh commented 2 years ago

@paralin which half? I know there is still the ts-proto side of this, if that is the half you're referring to, then agreed.

Otherwise, for the ts-poet project/issue in particular, is there something specific in ts-poet that is still wrong/needs improved?

paralin commented 2 years ago

@stephenh The hack I've put in place actually modifies some behavior in emitImports, I think there will need to be some more options added to emitImports that ts-proto uses to override the paths

stephenh commented 2 years ago

@paralin ah yeah, we're talking across ts-poet/ts-proto issues, but my assumption was that your further customization of emitImports is less of something that ts-poet would/should know how to do (i.e. ts-poet itself becoming aware of Go's vendoring notion and/or having to know which imports do/do not come from "this project" vs. "vendor directory"), and instead something that should probably be handled upstream in ts-proto.

My thought is that ts-proto is better setup to know "is this to-be-imported type 'in my current project' or 'from an external/vendored import'", and, then if so, also know "where is the external/vendored file actually at".

Hence thinking that this ts-poet issue itself is good/closed, and the "other half" would be handling in the ts-proto issue 597, and/or improvements on top of it.

paralin commented 2 years ago

@stephenh Agreed, just saying that some component of that logic will need to be added to ts-poet because the decision is per-import.