fix: add .js suffix to proto cross-reference imports #602

paralin · 2022-06-29T03:57:19Z

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

stephenh · 2022-06-29T04:06:55Z

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');


Huh! I didn't know that if you have a file called foo.ts, you're allowed to import it as foo.js.

That makes sense, I guess, since it will all wash out to *.js files in the end / post-compilation.

I dunno, that said, does it work to make this .ts?

Personally I think that'd be less surprising to the reader, since they would normally be reading the files as pre-tsc TS code. And I assume tsc will make all of the foo.ts imports into foo.js in the output anyway (?).

does it work to make this .ts?

No, this returns an error from the compiler (counter-intuitively)

Also huh!

Wdyt about doing a s,\./_/ on the file names, so that your myother.pb.ts files end up as myother_bs and imports like like:

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

It seems like this would be less disruptive to existing users that aren't use the .pb naming convention.

Granted, it sounds like file extensions are becoming defacto required in ESM, but hopefully/surely they've fixed/allow .ts extensions by then.

I'm very interested in using .pb.js since I'm using .pb.go - and with this patch it's working correctly.

We're going to have to use the .js suffix for all non-package imports with ESM anyway - (for example protobufjs/minimal.js) so eventually this will be required. And already the TypeScript compiler ignores the .js suffix and finds the .ts file, so it shouldn't be backwards incompatible, right?

If it causes an issue I guess it could be another option (to add .js as the suffix everywhere we try to import a ts file).

Yeah, totally get/agree we'll need it with ESM, albeit I'm ~80% sure that *.ts suffixes will be/are kosher with that tsconfig (I've only seen the TS release notes go by, haven't personally used it yet).

Maybe do a conditional here, like if importString has a . in it, add the .js export?

I'm just thinking, while this makes a ton of sense for you, for the other pre-ESM users of ts-proto, they might read the imports and just do an raised eyebrow about "why is that there?", even if I agree it won't break anything for them.

If the conditional-if-.-in-importString fixes your .pb.js use case, let's do that? But if not, yeah, I'm good with always having it.

I've tried importing with the .ts suffix and it was straight-up not allowed by the compiler. I suggest you give it a try and see for yourself. It throws errors as far as I know.

Ideally I'd love to move to using esm soon. Currently the only reason I'm still using the "module": legacy setting is ts-proto.

Oh damn, you're right. I had read this part of the release notes:

To overlay the way TypeScript works in this system, .ts and .tsx files now work the same way.

As ".ts files work in the same way" --> "can be imported with .ts/.tsx file extensions".

But you're right, further down:

As a result, it will have to be rewritten to use the extension of the _output_ of foo.ts – so bar.ts will instead have to import from ./foo.js.

Weird. Okay, yeah, this is the future 🤷 , so sgtm.

I agree it's confusing and at first when I saw the .js imports in files I was like.. wat?

But like you said, it's the way the TypeScript developers decided to go for this, and I guess it's slightly less ambiguous than guessing to add the .ts suffix.

With regards to importing with '.ts' suffix: microsoft/TypeScript#37582

Nice! Thanks for the link to the TS issue re .ts imports.

paralin · 2022-06-29T19:11:47Z

Probably can be done in a separate PR but this is also necessary if ESM is enabled:

import * as _m0 from 'protobufjs/minimal.js';

paralin · 2022-06-29T20:10:44Z

#603 (merged)

The TypeScript compiler will recognize that an import to foo.js is actually an import to foo.ts, if the .ts file exists and the .js file does not. If two import paths are the same, ignoring a ".js" or ".ts" or ".jsx" suffix, treat them as belonging to the same module. Related to: stephenh/ts-proto#602 Signed-off-by: Christian Stewart <[email protected]>

* chore(npmignore): add .circleci and .idea Signed-off-by: Christian Stewart <[email protected]> * chore(gitignore): add .log dir Signed-off-by: Christian Stewart <[email protected]> * fix: imports to .js and .ts are resolved to the same file The TypeScript compiler will recognize that an import to foo.js is actually an import to foo.ts, if the .ts file exists and the .js file does not. If two import paths are the same, ignoring a ".js" or ".ts" or ".jsx" suffix, treat them as belonging to the same module. Related to: stephenh/ts-proto#602 Signed-off-by: Christian Stewart <[email protected]>

The TypeScript compiler will recognize that an import to foo.js is actually an import to foo.ts, if the .ts file exists and the .js file does not. If two import paths are the same, ignoring a ".js" or ".ts" or ".jsx" suffix, treat them as belonging to the same module. Related to: stephenh/ts-proto#602 Signed-off-by: Christian Stewart <[email protected]>

paralin · 2022-07-01T23:00:11Z

@stephenh Fixed.

Ts-jest doesn't support the .js import alias yet, so it required adding resolver: 'jest-ts-webcompat-resolver' to the jest.config.js.

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 <[email protected]>

paralin · 2022-07-01T23:17:09Z

Okay, I think it's good to go now 👍🏽

## [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 · 2022-07-02T13:53:36Z

🎉 This PR is included in version 1.116.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Upstream PR was merged: stephenh/ts-proto#602 Signed-off-by: Christian Stewart <[email protected]>

zsmatrix62 · 2022-07-04T07:40:15Z

I think this change is making my error like below today: " Module not found: Error: Can't resolve '.xxxx.js' in ...

Are there any options to control the output just remove .js extension? .. im reverting the versions to use old libs...

paralin · 2022-07-04T09:46:23Z

@zsmatrix62 if you use typescript 4.7 or later, it will correctly resolve the .js to the .ts

zsmatrix62 · 2022-07-04T09:52:26Z

@zsmatrix62 if you use typescript 4.7 or later, it will correctly resolve the .js to the .ts

I'm using Angular13 which runs on ts 4.4 and only.

paralin force-pushed the fix-suffix-imports branch from bd9f6cc to 5fb9994 Compare June 29, 2022 04:00

stephenh reviewed Jun 29, 2022

View reviewed changes

paralin force-pushed the fix-suffix-imports branch from 5fb9994 to 74737c5 Compare June 29, 2022 19:09

paralin force-pushed the fix-suffix-imports branch from 74737c5 to 8c9d41c Compare July 1, 2022 21:32

paralin mentioned this pull request Jul 1, 2022

fix: imports to .js and .ts are resolved to the same file stephenh/ts-poet#35

Merged

paralin force-pushed the fix-suffix-imports branch 3 times, most recently from 1361c99 to 1a965ec Compare July 1, 2022 22:59

paralin force-pushed the fix-suffix-imports branch from 1a965ec to 420cd12 Compare July 1, 2022 23:00

paralin force-pushed the fix-suffix-imports branch from 420cd12 to 4db66bf Compare July 1, 2022 23:01

stephenh merged commit 8dc38af into stephenh:main Jul 2, 2022

stephenh added the released label Jul 2, 2022

paralin deleted the fix-suffix-imports branch July 2, 2022 19:58

paralin added a commit to aperturerobotics/starpc that referenced this pull request Jul 2, 2022

refactor: update ts-proto and drop merged patch

4987387

Upstream PR was merged: stephenh/ts-proto#602 Signed-off-by: Christian Stewart <[email protected]>

zsmatrix62 mentioned this pull request Jul 4, 2022

import "base.proto" to from "./base.js" #611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add .js suffix to proto cross-reference imports #602

fix: add .js suffix to proto cross-reference imports #602

paralin commented Jun 29, 2022 •

edited

Loading

stephenh Jun 29, 2022

paralin Jun 29, 2022

stephenh Jun 29, 2022

paralin Jun 29, 2022

stephenh Jun 29, 2022

paralin Jun 29, 2022 •

edited

Loading

stephenh Jun 29, 2022

paralin Jun 29, 2022

paralin Jul 1, 2022

stephenh Jul 2, 2022

paralin commented Jun 29, 2022

paralin commented Jun 29, 2022 •

edited

Loading

paralin commented Jul 1, 2022

paralin commented Jul 1, 2022

stephenh commented Jul 2, 2022

zsmatrix62 commented Jul 4, 2022 •

edited

Loading

paralin commented Jul 4, 2022

zsmatrix62 commented Jul 4, 2022

fix: add .js suffix to proto cross-reference imports #602

fix: add .js suffix to proto cross-reference imports #602

Conversation

paralin commented Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paralin Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paralin commented Jun 29, 2022

paralin commented Jun 29, 2022 • edited Loading

paralin commented Jul 1, 2022

paralin commented Jul 1, 2022

stephenh commented Jul 2, 2022

zsmatrix62 commented Jul 4, 2022 • edited Loading

paralin commented Jul 4, 2022

zsmatrix62 commented Jul 4, 2022

paralin commented Jun 29, 2022 •

edited

Loading

paralin Jun 29, 2022 •

edited

Loading

paralin commented Jun 29, 2022 •

edited

Loading

zsmatrix62 commented Jul 4, 2022 •

edited

Loading