timostamm / protobuf-ts

Protobuf and RPC for TypeScript
Apache License 2.0
1.06k stars 127 forks source link

Add file extensions to generated imports. #182

Open sachaw opened 2 years ago

sachaw commented 2 years ago

Files generated by the protoc plugin, currently, import witht he following syntax import {x} from 'y' however for reasons outlined in https://github.com/nodejs/modules/issues/444, the following syntax should be used for forward compatibility with esmodules import {x} from 'y.js'

Further reading: https://nodejs.org/api/esm.html#import-specifiers https://devblogs.microsoft.com/typescript/announcing-typescript-4-5-beta/

timostamm commented 2 years ago

Thanks for bringing this up, @sachaw. I am happy to see that TypeScript will support Node.js 12's ESM additions. I wonder what that means for users of older TypeScript versions, though. Just adding a .js extension will probably break existing code.

sachaw commented 2 years ago

I guess for now, it can be a flag, but considering how the world is moving towards ESM only, I think it will be the default quite soon.

sachaw commented 2 years ago

@timostamm I could make an attempt at implementing this, if you could lead me in the right direction. This is currently a blocker for me.

timostamm commented 2 years ago

I'm happy to. The following function creates import paths:

https://github.com/timostamm/protobuf-ts/blob/fbce12b7259cacea5d45ebf3503c8ac8a0200c73/packages/plugin-framework/src/typescript-imports.ts#L285-L288

The function is local to the typescript-imports.ts module (of the package @protobuf-ts/plugin-framework), and exports the class TypeScriptImports.

The only place the class is used is in the plugin, on this line: https://github.com/timostamm/protobuf-ts/blob/fbce12b7259cacea5d45ebf3503c8ac8a0200c73/packages/plugin/src/protobufts-plugin.ts#L161

So TypeScriptImports could take a boolean argument in its constructor, enabling the .js extension in imports. (Or, maybe better, make the .js extension the default behavior and let the argument disable it.)

To add a new plugin option, in protobufts-plugin.ts:

// add below the other option definitions:
enable_import_extension: {
    description: "...",
},
// further below:
imports = new TypeScriptImports(symbols, !params.enable_import_extension),
timostamm commented 2 years ago

There is also the deprecated packages/plugin-framework/src/typescript-import-manager.ts, which can be ignored - it is not used anymore.

There is a spec file for parts of typescript-imports.ts. It is probably best to extend the test coverage a bit (its not actually covering TypeScriptImports yet) and implement against it.

I don't think it will be feasible to add test coverage in @protobuf-ts/plugin, because if I'm not mistaken, it uses a TypeScript version that does not support the .js imports yet.

nvandamme commented 2 years ago

233

jan-dolejsi commented 9 months ago

Yes please. I have to write some post-processing step to add the .js extensions to these generated imports:

import { Duration } from "./google/protobuf/duration_pb.js";
import { Timestamp } from "./google/protobuf/timestamp_pb.js";

Or I cannot use it in some of my modules that stepped up. Without the dirty and somewhat dangerous post-processing script.