connectrpc / connect-es

The TypeScript implementation of Connect: Protobuf RPC that works.
https://connectrpc.com/
Apache License 2.0
1.32k stars 75 forks source link

Option to control file extensions in generated code #241

Closed dnsco closed 2 years ago

dnsco commented 2 years ago

This is very related to #236

Much like the above issue I'm on a create-react-app, am struggling to figure out how to get something working with CRA/typescript/jest (which has dubious support for modules)

It would be nice if I could stay on the happy path of not thinking too much about my webpack config.

I know it's in a weird sort-of-deprecated state, but as create react app is the blessed solution for bootstrapping a react app, it'd be nice if I could get this working more or less out of the box with it, and it seems like having an option to throw file extensions on the generated code is easier than coordinating between facebook + microsoft and getting all of their compilers to work together.

Thanks for the library, it's great so far!

dnsco commented 2 years ago

I originally couldn't get it to compile with target=ts, so I switched to js + d.ts, then I couldn't get jest to compile due to ECMA module support :/

I switched back to target=ts and used sed to s/_pb.js/_pb/ and hoped it would magically make everything compile, but alas. It makes sense that it wouldn't fix the underlying import issues but one can dream.

I see you are generating both cjs & esm in the dist, however it appears the generated code is always dependent on the ems.

Perhaps I should make another issue for allowing use of the cjs dist from generated code.

I'm gonna try and get jest/CRA working with esm again (I tried about a year ago and it was annoying), which will negate my personal need for an option to use the cjs dist in generated code, but I'm still wondering why the cjs exists if it effectively can't actually be used.

I figured I should document the onboarding issues I'm having as I'm taking a relatively standard approach to get here.

Thanks again for the library, looking forward to getting all this working.

The implementation is working, and is way nicer than the protobuf-gen-ts/grpc-web impl I was using before, now I just gotta get the tests passing.

smaye81 commented 2 years ago

Hey @dnsco. Have you checked out our Connect Web Integration repo? It has examples of using Connect-Web with a bunch of different frameworks. You can check out a working example of Create React App here.

The example is using target=ts and some Webpack overrides, just fyi. However, it will still work if you remove the target line from buf.gen.yaml (js+dts is the default target). I know you mentioned you didn't want to worry about the Webpack config much, so that would be my recommended route.

I feel your pain on getting a project setup. It is super tedious. Let me know if that example helps.

dnsco commented 2 years ago

@smaye81 thanks for pointing me in that direction– I got it set up with the typescriptresolver/tsjest, so I think I'm further along but...

trying to run the tests, just importing the generated code (and I believe not exectuting any grpc requests is)

ConnectError: [unknown] TextEncoder is not defined

not sure what's going on, but flailing publicly in the hopes that you know instantly.

dnsco commented 2 years ago

...and the above is apparently a jsdom issue,

throwing

import { TextEncoder } from "util";

global.TextEncoder = TextEncoder;

in my setupTests.ts seems to be getting me further

smaye81 commented 2 years ago

Yep, was just going to suggest that. We didn't encounter that issue in our setup. Did you add js-dom specifically? I don't believe that comes with the stock Create React App scaffolding.

smaye81 commented 2 years ago

@dnsco going to close this issue as we don't plan to add options to control the file extensions. The generated .js extensions are intentional. Hopefully you got things running. If you encounter another (unrelated) problem, feel free to open another issue.

dnsco commented 2 years ago

@smaye81 I got it running, but the barrier to entry felt pretty high– I'm through it but it feels like a bit of a funnel for future users.

jsdom is how jest, the CRA test runner runs tests, a bunch of the issues I had were not having this lib tank my test suite.

Now that i've dealt with it I'm gonna keep going (the library is great), but it is a bummer that there are orthogonal issues.

If you're gonna encode a hard-line, you must use esm opinion, you might want to stop generating the cjs code in dist.

timostamm commented 1 year ago

@dnsco, with the addition of support for Node.js, we have also added the plugin option import_extension to control this behavior.

It's documented around midway in this README. The plugin option is available for all plugins based on @bufbuild/protoplugin.

DanielEscorciaOrtiz commented 1 year ago

Although it is indeed mentioned in the readme, there is not an example of how to add it Searching through the organization repos I saw a config at bufbuild/connect-query. My working config look like this

version: v1
plugins:
  - name: es
    out: ./gen
    opt:
      - target=ts
  - name: connect-es
    out: ./gen
    opt:
      - target=ts
      - import_extension=none
timostamm commented 1 year ago

Thanks for the shout, @DanielEscorciaOrtiz. We'll update the readme.