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.

Sign up for GitHub

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

fix: conflicting option with nestJs and outputIndex #1018

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: conflicting option with nestJs and outputIndex #1018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions integration/nestjs-metadata-grpc-js/hero.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import { GrpcMethod, GrpcStreamMethod } from "@nestjs/microservices";
import * as _m0 from "protobufjs/minimal";
import { Observable } from "rxjs";

export const protobufPackage = "hero";

export interface HeroById {
id: number;
}
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-metadata-observables/hero.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ import { Metadata } from "@grpc/grpc-js";
import { GrpcMethod, GrpcStreamMethod } from "@nestjs/microservices";
import { Observable } from "rxjs";

export const protobufPackage = "hero";

export interface HeroById {
id: number;
}
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-metadata-restparameters/hero.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ import { Metadata } from "@grpc/grpc-js";
import { GrpcMethod, GrpcStreamMethod } from "@nestjs/microservices";
import { Observable } from "rxjs";

export const protobufPackage = "hero";

export interface HeroById {
id: number;
}
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-metadata/hero.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ import { Metadata } from "@grpc/grpc-js";
import { GrpcMethod, GrpcStreamMethod } from "@nestjs/microservices";
import { Observable } from "rxjs";

export const protobufPackage = "hero";

export interface HeroById {
id: number;
}
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-restparameters/hero.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
import { GrpcMethod, GrpcStreamMethod } from "@nestjs/microservices";
import { Observable } from "rxjs";

export const protobufPackage = "hero";

export interface HeroById {
id: number;
}
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-simple-observables/hero.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
import { GrpcMethod, GrpcStreamMethod } from "@nestjs/microservices";
import { Observable } from "rxjs";

export const protobufPackage = "hero";

export interface HeroById {
id: number;
}
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-simple-restparameters/google/protobuf/empty.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
/* eslint-disable */

export const protobufPackage = "google.protobuf";

/**
* A generic empty message that you can re-use to avoid defining duplicated
* empty messages in your APIs. A typical example is to use it as the request
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-simple-restparameters/hero.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ import { GrpcMethod, GrpcStreamMethod } from "@nestjs/microservices";
import { Observable } from "rxjs";
import { Empty } from "./google/protobuf/empty";

export const protobufPackage = "hero";

export interface User {
id: number;
name: string;
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-simple-usedate/google/protobuf/empty.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
/* eslint-disable */

export const protobufPackage = "google.protobuf";

/**
* A generic empty message that you can re-use to avoid defining duplicated
* empty messages in your APIs. A typical example is to use it as the request
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-simple-usedate/google/protobuf/timestamp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
/* eslint-disable */

export const protobufPackage = "google.protobuf";

/**
* A Timestamp represents a point in time independent of any time zone or local
* calendar, encoded as a count of seconds and fractions of seconds at
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-simple-usedate/hero.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import { wrappers } from "protobufjs";
import { Observable } from "rxjs";
import { Empty } from "./google/protobuf/empty";

export const protobufPackage = "hero";

export interface HeroById {
id: number;
}
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-simple/google/protobuf/empty.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
/* eslint-disable */

export const protobufPackage = "google.protobuf";

/**
* A generic empty message that you can re-use to avoid defining duplicated
* empty messages in your APIs. A typical example is to use it as the request
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-simple/google/protobuf/struct.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
/* eslint-disable */
import { wrappers } from "protobufjs";

export const protobufPackage = "google.protobuf";

/**
* `NullValue` is a singleton enumeration to represent the null value for the
* `Value` type union.
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-simple/google/protobuf/timestamp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
/* eslint-disable */

export const protobufPackage = "google.protobuf";

/**
* A Timestamp represents a point in time independent of any time zone or local
* calendar, encoded as a count of seconds and fractions of seconds at
Expand Down
2 changes: 0 additions & 2 deletions integration/nestjs-simple/hero.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import { Empty } from "./google/protobuf/empty";
import { Struct } from "./google/protobuf/struct";
import { Timestamp } from "./google/protobuf/timestamp";

export const protobufPackage = "hero";

export interface HeroById {
id: number;
}
Expand Down
3 changes: 2 additions & 1 deletion src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ export function generateFile(ctx: Context, fileDesc: FileDescriptorProto): [stri
const chunks: Code[] = [];

// Indicate this file's source protobuf package for reflective use with google.protobuf.Any
if (options.exportCommonSymbols) {
// since when nestJs=true, we have distinct protobuf package names we should exclude it here
if (options.exportCommonSymbols && !options.nestJs) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I wonder if we have to do it this way...

I think I would prefer keeping protobufPackage considered a "common symbol", and not give this specific nestJs handling.

And imo outputIndex should probably keep turning off exportCommonSymbols, even in nestJs, because the "common symbols" are usually ts-proto helper methods that would be defined in each file.

It seems like your core issue is that you'd like to revert:

https://github.com/stephenh/ts-proto/pull/916/files

Because your assertion is that, for you, the FOO_PACKAGE_NAME files are actually unique per file, and you want them in the barrel files.

My guess is that @eladhaim 's NestJS projects don't have this condition, so this ends up being a project-by-project assumption.

If so, I don't think we can know the right thing to do by just tweaking options.exportCommonSymbols & options.nestJs to try and guess your individual-but-opposite use cases.

So we'll probably need a new flag, like exportNestJsConsts that is usually true, but @eladhaim could then disable.

That way @TheMichio could leave exportCommonSymbols=false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @stephenh 👋
so sorry for late reply, been busy past couple of weeks.
I totally understand your point and you're right, it depends on the project, I can create the flag and set it up like you mentioned. will do it in a week or two and will let you know. see if you like it.
thanks thanks 😉

chunks.push(code`export const protobufPackage = '${fileDesc.package}';`);
}

Expand Down
4 changes: 3 additions & 1 deletion src/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,9 @@ export function optionsFromParameter(parameter: string | undefined): Options {
options.initializeFieldsAsUndefined = false;
}

if (options.outputIndex) {
// since when nestJs=true, we have distinct protobuf package names we should exclude it here
// otherwise, the common symbols won't be generated at all
if (options.outputIndex && !options.nestJs) {
options.exportCommonSymbols = false;
}

Expand Down
Loading