stephenh / ts-proto

An idiomatic protobuf generator for TypeScript
Apache License 2.0
2.13k stars 345 forks source link

Module protobufjs/minimal has no default export #536

Closed davidzhao closed 2 years ago

davidzhao commented 2 years ago

First of all, thanks for the fantastic work on this library. We found it to generate the cleanest set of typescript definitions and use it throughout livekit.

A few of our users have reported seeing the following when running generated scripts with Angular and Firebase:

Error: node_modules/livekit-client/dist/proto/livekit_models.d.ts:1:8 - error TS1192: Module '"projects/livekit/angular-issue/issue/node_modules/protobufjs/minimal"' has no default export.

1 import _m0 from "protobufjs/minimal";

One could workaround this issue with allowSyntheticDefaultImports, but it seems cleaner to generate the following instead

import * as _m0 from "protobufjs/minimal";
stephenh commented 2 years ago

@davidzhao Ah nice! Great to hear that ts-proto is working well for you; Livekit looks super impressive.

...per your suggestion, that sounds like a really good idea. I'm not sure why we didn't think of that before, b/c we've/I've been fighting the silly protobuf/minimal thing for awhile now, i.e. ts-proto has a flag to like conditionally change the protobufjs/minimal export based on whether the user is using the esModuleInterop flag...

Do you know if your suggestion of:

import * as _m0 from "protobufjs/minimal";

Would basically always work, regardless of the user's specific tsconfig? If so, that's pretty amazing. I'm currently futzing with the underlying ts-poet library to get it to generate this style of import (which it can do, but just needs a little polish), and so will plan on trying this out soon...

Thanks!

davidzhao commented 2 years ago

Reading a bit more on this, I think this format is "more" compatible?

import _m0 = require("protobufjs/minimal")

I've tested within our set up and it appears to work well.

stephenh commented 2 years ago

@davidzhao huh! I'd never seen that import-equal syntax before. I'm tempted to still go with the namespace-import; I've got a PR open in #540 and AFAICT it's working for both non-esModuleInterop (what the tests use by default) + the one test that does specifically use esModuleInterop.

Does that look good to you?

(I was kinda hoping this could kill off ts-proto's esModuleInterop flag all together, but it looks like long still needs it, although granted they've had an ESM-compliant release recently that we need to upgrade to...)

davidzhao commented 2 years ago

@stephenh neither have I! I was skeptical if mixing require is allowed. Testing showed that it is a valid syntax.

import * sounds good to me!

stephenh commented 2 years ago

@davidzhao great! Let's see how this goes, I merged #540 and the release should be up soon. Thanks!

davidzhao commented 2 years ago

awesome!! thank you for the quick fix. appreciate your work on this library!

stephenh commented 2 years ago

:tada: This issue has been resolved in version 1.110.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

Vincz commented 2 years ago

Hey guys! Actually, version 1.110.1 breaks my config with the following error:

if (_m0.util.Long !== Long) {
             ^
TypeError: Cannot read properties of undefined (reading 'Long')

I'm using esbuild to build my typescript project.

Here is my tsconfig:

{
    "compilerOptions": {
      "module": "es2015",
      "target": "esnext",
      "moduleResolution": "node",
      "sourceMap": true,
      "outDir": "dist",
    },
    "lib": ["es2015"]
  }

Here is my build script:

const { argv } = require('process');

require('esbuild')
    .build({
        entryPoints: ['./src/index.ts'],
        outdir: './dist',
        bundle: true,
        format: 'esm',
        platform: 'node',
        external: ['./node_modules/*'],
        ...(argv.includes('--watch')
            ? {
                  watch: {
                      onRebuild: () => console.log('Rebuilding...'),
                  },
              }
            : {}),
    })
    .catch(() => {
        process.exit(1);
    });

and here is how I generate my protobuff file:

protoc --plugin=./node_modules/.bin/protoc-gen-ts_proto --ts_proto_out=./src ./protocol/*.proto --ts_proto_opt=env=node --ts_proto_opt=esModuleInterop=true --ts_proto_opt=forceLong=long

It's working if I revert back to 1.110.0. Any idea?

leppaott commented 2 years ago

@stephenh this is broken as @Vincz pointed out. Had to revert back to 1.110 as well instead of updating to the latest. We only had a single file change on all proto files and it was this import star.

Also we had: "long": "^5.2.0",

stephenh commented 2 years ago

@Vincz / @leppaott shoot, yeah, sorry about this! The long library has been a PITA for us, because it was a really old CommonJS module that always required various esModuleInterop/etc. flags to use correctly (like ts-proto conditionally generates different esModuleInterop or non-esModuleInterop code, based on whether the user is using that tsconfig flag, solely b/c of the long library's fussiness).

I was excited to see that long finally got an ESM compatible release with version 5.x, about 9 months ago, but protobuf.js is still pinned to 4.0.0:

https://github.com/protobufjs/protobuf.js/blob/master/package.json#L51

So I assume ts-proto will have to stay pinned to long 4.x as well.

So @leppaott I think that is probably your issue, i.e. try using longjs 4.0.0?

@Vincz honestly I'm not really sure, and unfortunately don't have time to troubleshoot the long imports yet again. Very happy to accept a PR though if you find a fix. Thanks!

jonaskello commented 2 years ago

Upgraded from 1.97.0 to 1.115.4 and our codebase is now broken in the way described by @Vincz. So we need to downgrade again. If this is working for anyone else, could you please provide some info how to make it work?

Specifically we get this error in runtime (compile works fine):

if (_m0.util.Long !== Long) {
              ^
TypeError: Cannot read properties of undefined (reading 'Long')

For reference here are the flags we are using for ts-proto:

--ts_proto_opt=outputServices=generic-definitions,outputClientImpl=false,oneof=unions,snakeToCamel=false,esModuleInterop=true,useExactTypes=false
stephenh commented 2 years ago

Fwiw I poked around in the esModuleInterop integration test:

https://github.com/stephenh/ts-proto/tree/main/integration/simple-esmodule-interop

And can't get it to fail on the _m.util.Long line. Note that, for this integration test, I'm specifically compiling the simple.test.ts to .js and then running the test from the build/ to ensure its the emitted JS / __importStar / etc. code that's being exercised.

@jonaskello / @leppaott / @Vincz can you poke around at that test and try to reproduce the failure that you're seeing? If we can at least get it reproduced then we should be able to think through/evaluate fixes. Thanks!

aabluedragon commented 2 years ago

Bug still exists on version "1.121.0"

if (_m0.util.Long !== Long) {
        ^
TypeError: Cannot read properties of undefined (reading 'util')

Workaround: each time after generation, convert this line to import * as _m0 from 'protobufjs/minimal';

Hellomik2002 commented 1 year ago

Fwiw I poked around in the esModuleInterop integration test:

https://github.com/stephenh/ts-proto/tree/main/integration/simple-esmodule-interop

And can't get it to fail on the _m.util.Long line. Note that, for this integration test, I'm specifically compiling the simple.test.ts to .js and then running the test from the build/ to ensure its the emitted JS / __importStar / etc. code that's being exercised.

@jonaskello / @leppaott / @Vincz can you poke around at that test and try to reproduce the failure that you're seeing? If we can at least get it reproduced then we should be able to think through/evaluate fixes. Thanks!

try run npm run start:debug with https://gitlab.com/Hellomik/nestjs_and_grpc_test