Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add file extensions to generated imports. #182

Open
sachaw opened this issue Oct 28, 2021 · 7 comments
Open

Add file extensions to generated imports. #182

sachaw opened this issue Oct 28, 2021 · 7 comments
Labels
enhancement New feature or request

Comments

@sachaw
Copy link

sachaw commented Oct 28, 2021

Files generated by the protoc plugin, currently, import witht he following syntax import {x} from 'y' however for reasons outlined in nodejs/modules#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
Copy link
Owner

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
Copy link
Author

sachaw commented Oct 28, 2021

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
Copy link
Author

sachaw commented Nov 6, 2021

@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
Copy link
Owner

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

* Create a relative path for an import statement like
* `import {Foo} from "./foo"`
*/
function createRelativeImportPath(currentPath: string, pathToImportFrom: string): string {

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:

imports = new TypeScriptImports(symbols),

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
Copy link
Owner

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.

@timostamm timostamm added the enhancement New feature or request label Nov 20, 2021
@nvandamme
Copy link

#233

@jan-dolejsi
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants