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 .js suffix to imports #601

Closed
paralin opened this issue Jun 29, 2022 · 14 comments · Fixed by #602
Closed

Add .js suffix to imports #601

paralin opened this issue Jun 29, 2022 · 14 comments · Fixed by #602
Labels

Comments

@paralin
Copy link
Collaborator

paralin commented Jun 29, 2022

When importing a cross-referenced file, a line like this is generated:

import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

import { MyMessage } from '../myother/myother.pb.js'

And this is also required if we are using esm mode.

The fix:

diff --git a/src/utils.ts b/src/utils.ts
index 60efc9e..a666e04 100644
--- a/src/utils.ts
+++ b/src/utils.ts
@@ -203,6 +203,6 @@ export function impProto(options: Options, module: string, type: string): Import
   if (options.onlyTypes) {
     return imp('t:' + importString);
   } else {
-    return imp(importString);
+    return imp(importString + '.js');
   }
 }
paralin added a commit to paralin/ts-proto that referenced this issue Jun 29, 2022
When importing a cross-referenced file, a line like this is generated:

import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

import { MyMessage } from '../myother/myother.pb.js'

Fixes stephenh#601

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/ts-proto that referenced this issue Jun 29, 2022
When importing a cross-referenced file, a line like this is generated:

import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

import { MyMessage } from '../myother/myother.pb.js'

Fixes stephenh#601

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/ts-proto that referenced this issue Jun 29, 2022
When importing a cross-referenced file, a line like this is generated:

import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

import { MyMessage } from '../myother/myother.pb.js'

Fixes stephenh#601

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/ts-proto that referenced this issue Jun 30, 2022
When importing a cross-referenced file, a line like this is generated:

import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

import { MyMessage } from '../myother/myother.pb.js'

Fixes stephenh#601

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/ts-proto that referenced this issue Jul 1, 2022
When importing a cross-referenced file, a line like this is generated:

import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

import { MyMessage } from '../myother/myother.pb.js'

Fixes stephenh#601

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/ts-proto that referenced this issue Jul 1, 2022
When importing a cross-referenced file, a line like this is generated:

import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

import { MyMessage } from '../myother/myother.pb.js'

Fixes stephenh#601

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/ts-proto that referenced this issue Jul 1, 2022
When importing a cross-referenced file, a line like this is generated:

import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

import { MyMessage } from '../myother/myother.pb.js'

Fixes stephenh#601

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/ts-proto that referenced this issue Jul 1, 2022
When importing a cross-referenced file, a line like this is generated:

  import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

  import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

  import { MyMessage } from '../myother/myother.pb.js'

Fixes stephenh#601

Fix the resolution in ts-jest with the jest-ts-webcompat-resolver:
See: kulshekhar/ts-jest#1057 (comment)

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/ts-proto that referenced this issue Jul 1, 2022
When importing a cross-referenced file, a line like this is generated:

  import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

  import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

  import { MyMessage } from '../myother/myother.pb.js'

Fixes stephenh#601

Fix the resolution in ts-jest with the jest-ts-webcompat-resolver:
See: kulshekhar/ts-jest#1057 (comment)

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/ts-proto that referenced this issue Jul 1, 2022
When importing a cross-referenced file, a line like this is generated:

  import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

  import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

  import { MyMessage } from '../myother/myother.pb.js'

Fixes stephenh#601

Fix the resolution in ts-jest with the jest-ts-webcompat-resolver:
See: kulshekhar/ts-jest#1057 (comment)

Signed-off-by: Christian Stewart <christian@paral.in>
paralin added a commit to paralin/ts-proto that referenced this issue Jul 1, 2022
When importing a cross-referenced file, a line like this is generated:

  import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

  import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

  import { MyMessage } from '../myother/myother.pb.js'

Fixes stephenh#601

Fix the resolution in ts-jest with the jest-ts-webcompat-resolver:
See: kulshekhar/ts-jest#1057 (comment)

Signed-off-by: Christian Stewart <christian@paral.in>
stephenh pushed a commit that referenced this issue Jul 2, 2022
When importing a cross-referenced file, a line like this is generated:

  import { MyMessage } from '../myother/myother'

.. assuming there's a proto at ../myother/myother.proto

But if we add the suffix '.pb.ts' to the generated files:

  import { MyMessage } from '../myother/myother.pb'

Is not recognized as a TypeScript import by tsc because of the .pb suffix.

To fix this, we can just add .js, and the TypeScript compiler recognizes that we actually mean the .ts file:

  import { MyMessage } from '../myother/myother.pb.js'

Fixes #601

Fix the resolution in ts-jest with the jest-ts-webcompat-resolver:
See: kulshekhar/ts-jest#1057 (comment)

Signed-off-by: Christian Stewart <christian@paral.in>
stephenh pushed a commit that referenced this issue Jul 2, 2022
## [1.116.1](v1.116.0...v1.116.1) (2022-07-02)

### Bug Fixes

* add .js suffix to proto cross-reference imports ([#602](#602)) ([8dc38af](8dc38af)), closes [#601](#601) [/github.com/kulshekhar/ts-jest/issues/1057#issuecomment-481406624](https://github.com//github.com/kulshekhar/ts-jest/issues/1057/issues/issuecomment-481406624)
@stephenh
Copy link
Owner

stephenh commented Jul 2, 2022

🎉 This issue has been resolved in version 1.116.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@xpt1x
Copy link

xpt1x commented Jul 3, 2022

This breaks code during compilation on typescript project ( projects create with CRA with eslint integrated )
This creates (for ex)

import { SomeInterface } from "./types.js";

This path (including .js) is not resolved by compiler as there is no such file, it expects

import { SomeInterface } from "./types";
OR
import { SomeInterface } from "./types.ts";

If i can override this with any config setting please let me know

@paralin
Copy link
Collaborator Author

paralin commented Jul 3, 2022

@xpt1x the typescript compiler expects the .js suffix and will resolve it to the .ts file automatically.

Maybe you're using an old version of TypeScript?

...And as we mentioned above importing the .ts directly doesn't work in the majority of cases.

@yoshiko-pg
Copy link

I have the same issue.
I am using the latest version of TypeScript, 4.7.4, and I get .js loading errors when building Next.js and running tests in Jest.
I would prefer the option to not include the extension.

@paralin
Copy link
Collaborator Author

paralin commented Jul 4, 2022

@yoshiko-pg I'd like to figure out why, because many projects are using the .js import, mine included, not in esmodule mode, and it works fine.

I guess it's the resolver part, depending on which resolver you have enabled...

I'll have a look at adding a flag to control it.

@xpt1x
Copy link

xpt1x commented Jul 4, 2022

@xpt1x the typescript compiler expects the .js suffix and will resolve it to the .ts file automatically.

Maybe you're using an old version of TypeScript?

...And as we mentioned above importing the .ts directly doesn't work in the majority of cases.

I am currently on typescript 4.4.4

I believe by default TS compiler only allows .ts /.tsx / .d.ts files

You must be having allowJs: true in ur tsconfig
https://www.typescriptlang.org/tsconfig#allowJs

@paralin
Copy link
Collaborator Author

paralin commented Jul 4, 2022

@xpt1x no, that's not set. TypeScript resolves the .js extension in the import to .ts automatically.

@paralin
Copy link
Collaborator Author

paralin commented Jul 4, 2022

@xpt1x please update to typescript 4.7 or later.

@stephenh
Copy link
Owner

stephenh commented Jul 4, 2022

@paralin from what @yoshiko-pg reports, it sounds like it's not just the latest TypeScript that users need; there are other import-aware tools (Next.js and Jest) that don't know the .js convention yet.

I'm pretty convinced we need to flag the new behavior, with one of the following options:

  • a) Restore old behavior, add a useJsExtension flag that you could enable
  • b) Restore old behavior, detect . (i.e. .pb) extensions in imports, and auto-add .js suffix only for them
  • c) Restore old behavior, add a esm=true flag flag that, when true, uses the .js extension + in theory would enable any other ESM-compliant changes we might need (my hesitation with this is that I believe protobuf.js moving to long.js 5.0.0 is a blocker for ts-proto to have robust ESM support)
  • d) ...something else...?

Not to nit-pick, but to support my "restore old behavior" push, I believe when you said:

And as we mentioned above importing the .ts directly doesn't work in the majority of cases.

AFAIU, that's not accurate insofar as IMO importing extension-less paths actually works in the (far?) majority of cases, at least in the ecosystem today, and it's only the (probably very small) minority of paths that use the .pb.ts syntax that don't work.

Granted, you meant have strictly meant "imports with the *.ts suffix", which I initially assumed "surely would work", in which case, right, we need to wait for TS to actually support that, but until then I'm thinking that extension-less paths should still be our default.

I know your strongest opinion is likely to keep the js suffix, but if you have a 2nd opinion after that one :-) on your preferred a/b/c/d above, and wouldn't mind opening a PR for it, I'd really appreciate it.

Personally I think I'd probably go with b), b/c it avoids "yet another flag" syndrome with ts-proto definitely suffers from, and I think still supports your use case, and kinda punts on "official ESM flag/support" until we're ready to do that.

@stephenh
Copy link
Owner

stephenh commented Jul 4, 2022

I'll have a look at adding a flag to control it.

Ah shoot, @paralin I was catching up on comments and missed this one; that sounds great, thanks!

@paralin
Copy link
Collaborator Author

paralin commented Jul 4, 2022

@stephenh The main concern I have is related to esm, almost all projects are now using .js imports: https://github.com/libp2p/js-libp2p-mplex/blob/master/src/index.ts#L3

Importing with .ts extension is not allowed. This is the error that typescript throws:

image

So the best option is to add a useJsExtension flag which adds .js to any directly-imported .ts file. In my opinion.

@paralin
Copy link
Collaborator Author

paralin commented Jul 4, 2022

@stephenh maybe a string option - importSuffix - default to none but allow js as an option?

@yoshiko-pg
Copy link

The problem has been completely resolved in v1.117.0. Thanks for the quick turnaround!

@paralin
Copy link
Collaborator Author

paralin commented Jul 14, 2022

Regarding importing .ts files directly: microsoft/TypeScript#37582 (comment)

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

Successfully merging a pull request may close this issue.

4 participants