tatethurston / TwirpScript

A protobuf RPC framework for JavaScript and TypeScript
MIT License
142 stars 13 forks source link

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

Closed andrewbeckman closed 2 years ago

andrewbeckman commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago
Screen Shot 2022-08-23 at 3 54 30 PM

TypeScript version: 4.5.4

andrewbeckman commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

tatethurston commented 2 years ago

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

tatethurston commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Alright I chased this down and the issue is a difference between the TypeScript compiler and webpack: https://github.com/TypeStrong/ts-loader/issues/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 commented 2 years ago

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

andrewbeckman commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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