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

Cannot use classes in protos #1496

Closed
orgads opened this issue Dec 1, 2023 · 7 comments · Fixed by #1498 or #1499
Closed

Cannot use classes in protos #1496

orgads opened this issue Dec 1, 2023 · 7 comments · Fixed by #1498 or #1499
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@orgads
Copy link
Contributor

orgads commented Dec 1, 2023

Environment details

  • Programming language: Typescript
  • OS: Linux
  • Language runtime version: 20
  • Package version: 4.2.0

Steps to reproduce

When esm output is used, the import from protos.js is using import type, and this doesn't allow using the concrete classes in the namespaces, or other values that are not types.

This is example is based on @google-cloud/speech package, in which I only replaced the following line in index.d.ts to import type *...:

import * as protos from '../protos/protos';

Then in my code I have:

import { v1, v1p1beta1, protos as googleProtos } from '@google-cloud/speech';
// An import alias cannot reference a declaration that was imported using 'import type'.ts(1380)
import protos = googleProtos.google.cloud.speech;
function test() {
  // 'protos' cannot be used as a value because it was imported using 'import type'.ts(1361)
  const foo = new protos.v1.SpeechContext();
  // 'protos' cannot be used as a value because it was imported using 'import type'.ts(1361)
  return protos.v1.RecognitionConfig.AudioEncoding.LINEAR16;
}

Is there a reason not to use a simple import instead?

@orgads orgads added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Dec 1, 2023
orgads added a commit to orgads/gapic-generator-typescript that referenced this issue Dec 1, 2023
When `import type` is used, it is not possible to use the classes
and values in the protos namespace.

Fixes googleapis#1496
@alexander-fenster
Copy link
Contributor

@sofisl can you take a look at this issue please?

@sofisl
Copy link
Contributor

sofisl commented Dec 2, 2023

Hi @orgads, what do you mean by when esm output is used? We don't currently publish our packages in esm, although we do have plans to do so soon! In order to publish in esm, we need to do a lot of changes, and we're about to experiment with @google-cloud/tasks.

I'm not sure I understand what this means either: Is there a reason not to use a simple import instead? Where are you referring to?

@orgads
Copy link
Contributor Author

orgads commented Dec 2, 2023

I refer to the templates. See #1498 for my fix.

I only used @google-cloud/speech as a reference. I generated another package using esm output, and in index.d.ts the import of protos is import type. This is also wrong, since the js/cjs files do import the protos, while import type discards it in the js output.

@sofisl
Copy link
Contributor

sofisl commented Dec 6, 2023

Gotcha! I think this is tied to another change that we made to the compileProtos step, which generates protos.js as es6 (vs. amd).

In your experiment, try changing this line to:

compileProtos esm/src --esm

Could you let me know if that works for you?

@orgads
Copy link
Contributor Author

orgads commented Dec 6, 2023

Once again, there is no problem with the google speech package. Please refer to my PR #1498 and my previous comment.

@sofisl
Copy link
Contributor

sofisl commented Dec 7, 2023

Hi @orgads, that line doesn't change anything in the speech package, it changes how the protos are compiled. It compiles the protos.js file into es6.

@orgads
Copy link
Contributor Author

orgads commented Dec 7, 2023

I know that. My problem is that the generated code is incorrect. Forget about the speech package.

orgads added a commit to orgads/gapic-generator-typescript that referenced this issue Dec 14, 2023
When `import type` is used, it is not possible to use the classes
and values in the protos namespace.

Fixes googleapis#1496
sofisl pushed a commit that referenced this issue Dec 15, 2023
When `import type` is used, it is not possible to use the classes
and values in the protos namespace.

Fixes #1496
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
3 participants