connectrpc / connect-query-es

TypeScript-first expansion pack for TanStack Query that gives you Protobuf superpowers.
https://connectrpc.com/docs/web/query/getting-started
Apache License 2.0
244 stars 17 forks source link

ES Module shipped in a CommonJS package? #17

Closed mckelveygreg closed 1 year ago

mckelveygreg commented 1 year ago

Hey! Love the direction of this library and just did a huge refactor to convert all of our grpc calls to using connect query!

I'm in the middle of fixing up our tests and got this message from vistest:

Module /Users/greg/code/phc/location-fe/node_modules/@bufbuild/connect-query/dist/src/index.js:14 seems to be an ES Module but shipped in a CommonJS package. You might want to create an issue to the package "@bufbuild/connect-query" asking them to ship the file in .mjs extension or add "type": "module" in their package.json.

I'm not sure what the best path forward for you lib is, but I thought I would let you know about the warning.

Vitest also posted a work around, so I think I'm fine for now:

As a temporary workaround you can try to inline the package by updating your config:

// vitest.config.js
export default {
  test: {
    deps: {
      inline: [
        "@bufbuild/connect-query"
      ]
    }
  }
}

Cheers and thanks for all the work! 🎉

mckelveygreg commented 1 year ago

Oh dear... not sure why this happened actually. Sorry, i may have posted prematurely, the error message was so confident!

I used this library to make an internal library for our product, but node_modules may have gotten dirty during development. Cleaned everything out and it seems to be working as it should.

sorry for the noise!

mckelveygreg commented 1 year ago

I guess this issue found its way to me again, but this time in the node side of things.

For context, we are generating a library from our protos for both connect-web and connect-query in the same repo. This became problematic when the node service (which is full esm) needed to import bits.

It appears fixed for us by just adding "type": "module" to the package.json, but I don't fully understand the implications of doing that...

The connect-web library is causing no issues 🤷

dimitropoulos commented 1 year ago

for sure: yes. I put up a PR, but this seems like something that's reasonable to ship soon. Should have been there in the first place, really. Thanks for your patience! The team was at JS World, so hence the silence. Just got back today, though, so we're right on it!

mckelveygreg commented 1 year ago

Learning how ESM works has been... tough 🙃 But we are in the process of adopting Buf everywhere we can, so this has been exciting to try things out as they are built!

sher commented 1 year ago

@dimitropoulos Is there any progress here? We are facing the same issue and vitest inlining is not helping.

dimitropoulos commented 1 year ago

@sher @mckelveygreg (and anyone else watching) please see https://github.com/bufbuild/connect-query/pull/20#issuecomment-1507501612. TLDR; if you could try out an alpha release (0.2.0-alpha.0) we cut for this real quick it would help ensure our fix worked.

nickadamson commented 1 year ago

@dimitropoulos chiming in that using 0.2.0-alpha.0 and setting this nextjs config value resolved the ESM/CJS issues I was having. Not exactly relevant to vite but hoping it may help someone else.

sher commented 1 year ago

@nickadamson have you tried running tests after you set @bufbuild/connect-query in transpilePackages? In our case, after transpiling/inlining the package <TransportProvider /> was not able to pass the custom transport down to the child hooks. So every call to fooBar.useQuery required an explicit transport setting, which works, but should pick one from TransportProvider.

mckelveygreg commented 1 year ago

Thanks for working through this! Finally got some time to update our own libs and wanted to check back in.

I was still running into an issue until in my own client lib that building, I made the exports use the .js extension, then everyone is happy 🤷

Thanks again for all your work!