proto-loader-gen-types: Import internal files with extension

[atjn]

Different runtimes have different requirements for which extension you need to put on relative file imports. It is problematic that the type generator only supports bare imports. This is for example not supported in Deno.

This PR adds an option to specify which file extension should be used.

I have tested the changes in my own repo, and everything works for me.
Closes #2401

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: atjn / name: Anton (8d69e9f, 83d9f1c, 055282a)

[murgatroid99]

I don't really understand this. TypeScript is a compiled language. The runtime shouldn't care about the raw TypeScript content, and if it doesn't like the compiler output, that should be fixable by changing the compiler options.

Edit: I see that Deno supports TypeScript natively. So my objection is more that this code is valid TypeScript according to the TypeScript compiler, so it should be valid TypeScript everywhere. I dislike the idea of catering to variants like this because that is potentially an endless rabbit hole of compatibility code.

[atjn]

It is only valid typescript if you use legacy settings. Modern node resolution settings require that you use .js extensions for relative import of typescript files.

From the TS docs: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-7.html#type-in-packagejson-and-new-extensions

Relative import paths need full extensions (we have to write import "./foo.js" instead of import "./foo").

.js is chosen to be compatible with node, which would not understand what a .ts file is. This choice is different in other runtimes that have native typescript support:

From the Deno docs: https://docs.deno.com/runtime/manual/advanced/typescript/overview#determining-the-type-of-file

When the extension is absent in a local file, it is assumed to be JavaScript.

That quote means if you try to import a typescript file with no extension, it will be read by a javascript runtime which will fail because it cannot read typescript. There is no option to change this configuration, you must use .ts at the end of files.

..so TL;DR if you don't support this feature, your code cannot run in modern Node projects and can not run in Deno at all. I suspect other runtimes will be unsupported as well.

[murgatroid99]

If I understand that TypeScript update page correctly, the extension should always be .ts because the imports are purely type imports. I would support that change, if it doesn't break anything.

[atjn]

I am not sure how you could come to that conclusion. If you use the .ts extension in Node, you need to significantly change your compiler config to support that use case. As far as I can tell, it is impossible to even run it without importing third party software that can read and understand the .ts extension and transform it into something that Node can run.

I proposed this change because it would be a simple change with no breakage or refactoring. However if you are looking for the most correct solution to this, I think it is to use .d.ts extensions for all the files, and also importing them with the .d.ts extensions. Files with that extension only allow types which are removed at compile time, and therefore typescript does not need to be Node compliant. This will be a breaking change though because I am sure somebody is relying on the extension to be .ts, and will have breakage when all the files change to .d.ts. I also am not sure if this tool can be used to produce typescript that is not allowed in a .d.ts file.

[murgatroid99]

Node should have no involvement here. These are type-only imports of type-only files. They should be completely omitted in any compiled code that Node would see.

[atjn]

I think I understand now. It seems you are correct, if you use the type declarator to show that you are only doing type imports, then .ts extensions are also supported in Node. I missed the point on that, sorry :)

So what you are saying is that you prefer to always use the .ts extension and you don't want an option to choose/toggle the extension? I don't know if that breaks some backwards compatibility, but I doubt it.

[murgatroid99]

I would say that it's not so much that this is supported on Node, as that it is never executed on Node.

It looks like it is possible for this code to use a single extension that works everywhere, and if so, we should just do that. Then providing an option to use a different extension is superfluous.

[atjn]

Should be ready for merge now :)

[murgatroid99]

Please run the generate-golden npm script in the proto-loader package.

[atjn]

I also updated the example in the docs to use the same import syntax :)

[murgatroid99]

Thank you for your contribution.

[atjn]

Thanks for helping me get this merged!

[murgatroid99]

This is now out in version 0.7.11

[versat]

Yesaterday we suddenly observed new eslint errors in our (unchanged, only rebuilt) code. It took us some time to find out that @grpc/proto-loader 0.7.11 caused it.
Errors look like this:

D:\project\src\grpcClient.ts
  339:9   error  Unsafe assignment of an `any` value              @typescript-eslint/no-unsafe-assignment
  521:36  error  Unsafe member access .version on an `any` value  @typescript-eslint/no-unsafe-member-access

D:\project\src\grpcCommandHandler.ts
  37:15  error  Unsafe member access .serverPort on an `any` value                                            @typescript-eslint/no-unsafe-member-access
  38:55  error  Invalid type "any" of template literal expression                                             @typescript-eslint/restrict-template-expressions
  38:55  error  Unsafe member access .serverPort on an `any` value                                            @typescript-eslint/no-unsafe-member-access
  39:21  error  Unsafe argument of type `any` assigned to a parameter of type `number | PromiseLike<number>`  @typescript-eslint/no-unsafe-argument
  39:21  error  Unsafe member access .serverPort on an `any` value                                            @typescript-eslint/no-unsafe-member-access
  40:22  error  Unsafe member access .errorCode on an `any` value                                             @typescript-eslint/no-unsafe-member-access
  41:55  error  Invalid type "any" of template literal expression                                             @typescript-eslint/restrict-template-expressions
  41:55  error  Unsafe member access .errorCode on an `any` value                                             @typescript-eslint/no-unsafe-member-access

This is how we are calling proto-loader-gen-types:
npx proto-loader-gen-types --grpcLib=@grpc/grpc-js --outDir=src/api/ ../../grpc/*.proto

These are the most relevant parts for importing the grpc stuff:

import * as grpc from '@grpc/grpc-js';
import * as protoLoader from '@grpc/proto-loader';

import { ProtoGrpcType as DMErrorCode } from './api/dm_errorcode';
import { ProtoGrpcType as UEDM } from './api/UEDM';
import { UEDMClient } from './api/UE/DM/UEDM';

I guess we have to change something so the types are known again? But I am not sure what exactly. Do you have any idea?
Currently our workaround is to lock @grpc/proto-loader to 0.7.10, but this is not a solution.

[atjn]

@versat have you tried importing the types as types?

-import { ProtoGrpcType as DMErrorCode } from './api/dm_errorcode';
+import type { ProtoGrpcType as DMErrorCode } from './api/dm_errorcode.ts';

-import { ProtoGrpcType as UEDM } from './api/UEDM';
+import type { ProtoGrpcType as UEDM } from './api/UEDM.ts';

-import { UEDMClient } from './api/UE/DM/UEDM';
+import type { UEDMClient } from './api/UE/DM/UEDM.ts';

[versat]

I have adapted the imports, but now I am getting even more of these errors.
Currently we are using typescript 4.9.5.
I have updated it to 5.4.3 because I saw that you created a PR for updating Typescript in this repository to fix something.
And with Typescript 5.4.3 eslint is happy now too :)
So, is it a hard requirement (now) to use Typescript >= 5?
Maybe this can be added as a requirement?

[murgatroid99]

I'm reverting this. It shouldn't be a minor change if it breaks compatibility with TypeScript 4. We can re-evaluate what the best approach is after that.

[atjn]

@murgatroid99 I can make a boolean that enables the .ts extension. You can leave it off by default, then enable it by default and deprecate it in the next major version.

[murgatroid99]

I think it could also be that a .js extension will work everywhere. I would want to get a better understanding of the practical consequences of these import statement variations before choosing a specific solution.