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

Generating TypeScript files from proto files that import other proto files is leading to js imports #192

Closed
andrewbeckman opened this issue Aug 23, 2022 · 18 comments

Comments

@andrewbeckman
Copy link

I have a proto file with the following import line:

import "product/models.proto";

And in the generated .pb.ts file, there's the following line:

import { UUID, UUIDJSON } from "../proto/uuid/models.pb.js";

My twirp config is as follows:

{
  "root": "../rd",
  "exclude": ["^(?!rpc).+"],
  "dest": "src/generated",
  "language": "typescript"
}

Should the generated import statement be importing from "../proto/uuid/models.pb.ts" or even just "../proto/uuid/models.pb" given that the generated file being imported is a ts file not a js file? I am currently running into blocking errors trying to build the generated code with Next.js because of this. I can try and work around those build issues, but it seems like this may genuinely be unintended.

@tatethurston
Copy link
Owner

tatethurston commented Aug 23, 2022

Hey @andrewbeckman the full extension path is expected — it’s required for ES modules which don’t implicitly append a js extension like commonjs does.

The TypeScripts compiler expects js extensions and not ts extensions because the compiler does not manipulate import paths: https://www.typescriptlang.org/docs/handbook/esm-node.html

What is the error you’re encountering? And what version of TypeScript are you using?

@andrewbeckman
Copy link
Author

Screen Shot 2022-08-23 at 3 54 30 PM

TypeScript version: 4.5.4

@andrewbeckman
Copy link
Author

Here's my tsconfig in case it helps

{
  "compilerOptions": {
    "target": "ES2020",
    "lib": ["dom", "dom.iterable", "esnext"],
    "allowJs": true,
    "skipLibCheck": true,
    "strict": false,
    "forceConsistentCasingInFileNames": true,
    "noEmit": true,
    "esModuleInterop": true,
    "module": "esnext",
    "moduleResolution": "node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "jsx": "preserve",
    "incremental": true,
    "baseUrl": "src",
    "strictNullChecks": true,
    "strictFunctionTypes": false,
    "downlevelIteration": true,
    "noUnusedLocals": true /* Report errors on unused locals. */,
    "noUnusedParameters": true /* Report errors on unused parameters. */
  },
  "include": [
    "src/**/*.ts",
    "src/**/*.tsx",
    "next-env.d.ts",
    "types/*.d.ts",
    "jest.setup.ts"
  ],
  "exclude": ["node_modules", "src/generated"]
}

@tatethurston
Copy link
Owner

Thanks @andrewbeckman, I'll look into this. There is likely a rough edge here, sorry about that. NextJS builds commonjs for the server build and esm for the client, so it sits right in the middle of a difficult transitional period for the JS ecosystem.

In the meantime, you may be able to resolve this by updating your tsconfig compilerOptions.moduleResolution to nodenext/ node16.

@andrewbeckman
Copy link
Author

Okay thanks @tatethurston. I had already tried updating the moduleResolution to both those values and unfortunately that introduced more issues.

@tatethurston
Copy link
Owner

tatethurston commented Aug 24, 2022

Cool, worth a try. Yeah, any project imports that aren't valid ESM would fail under the nodenext setting.

@tatethurston
Copy link
Owner

@andrewbeckman Could you update to TS 4.7 (4.7.4 is the latest version) and let me know if that resolves the issue for you?

https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#ecmascript-module-support-in-node-js

@andrewbeckman
Copy link
Author

@andrewbeckman Could you update to TS 4.7 (4.7.4 is the latest version) and let me know if that resolves the issue for you?

https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#ecmascript-module-support-in-node-js

I had already tried this as well. Unfortunately, same issue.

@tatethurston
Copy link
Owner

tatethurston commented Aug 24, 2022

Does tsc compile successfully in your project? I may need to look at Next's build process. I just tested the following TypeScript compiler config:

{
  "compilerOptions": {
    "target": "ES2020",
    "module": "esnext",
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true /
    "skipLibCheck": true 
  }
}

@andrewbeckman
Copy link
Author

Does tsc compile successfully in your project? I may need to look at Next's build process. I just tested the following TypeScript compiler config:

{
  "compilerOptions": {
    "target": "ES2020",
    "module": "esnext",
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true /
    "skipLibCheck": true 
  }
}

Yes! tsc compiles fine when I run yarn tsc.

@tatethurston
Copy link
Owner

Awesome. I suspect this is an issue with next’s build process. In particular, I suspect they’re using a babel plugin for ts compilation and it doesn’t have the same behavior as the TS compiler.

@tatethurston
Copy link
Owner

tatethurston commented Aug 26, 2022

Alright I chased this down and the issue is a difference between the TypeScript compiler and webpack: TypeStrong/ts-loader#1383

Webpack introduced extensionAlias to solve this problem: https://webpack.js.org/configuration/resolve/#resolveextensionalias

Here's an example of wiring this up: https://github.com/tatethurston/TwirpScript/pull/195/files#diff-02b18277d8fea71eaebddca674b4e25b357dcc8acb1feebfcc3573d015efb989

It looks like the TypeScript team may reevaluate this in 4.9

@tatethurston
Copy link
Owner

LMK if this doesn't resolve the issue for you @andrewbeckman, and 🤞 that this step can be eradicated once TS 4.9 lands.

@andrewbeckman
Copy link
Author

LMK if this doesn't resolve the issue for you @andrewbeckman, and 🤞 that this step can be eradicated once TS 4.9 lands.

Thanks for hunting this down. I saw this and looked into modifying the webpack config and Next seems to allow it though it discourages it.

@tatethurston
Copy link
Owner

tatethurston commented Aug 28, 2022

Yep: https://nextjs.org/docs/api-reference/next.config.js/custom-webpack-config. I wouldn’t be concerned about risk here. The caution is intended for things that depend on or would interfere with Nextjs’ webpack configuration, which this does not.

module.exports = {
  webpack: (config ) => {
    config.resolve.extensionAlias = {
      ".js": [".ts", ".js"]
    };
    return config;
  },
}

@tatethurston
Copy link
Owner

@andrewbeckman I've released https://github.com/tatethurston/TwirpScript/releases/tag/v0.0.64 which removes the need for the extensionAlias workaround we discussed earlier.

@andrewbeckman
Copy link
Author

@andrewbeckman I've released https://github.com/tatethurston/TwirpScript/releases/tag/v0.0.64 which removes the need for the extensionAlias workaround we discussed earlier.

@tatethurston Thanks for the heads up! We ended up making the switch to https://github.com/timostamm/protobuf-ts as we didn't want to go down the path of customizing our webpack config. Next time I have some time on my hands, will be sure to check out how things have progressed and decide if we should switch back!

@tatethurston
Copy link
Owner

Related: TwirpScript now includes a NextJS example: https://github.com/tatethurston/twirpscript/tree/main/examples/nextjs

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

No branches or pull requests

2 participants