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

[TS/JS] Allow --ts-flat-files and --gen-all to work in combination #7445

Closed
dbaileychess opened this issue Aug 15, 2022 · 6 comments
Closed

Comments

@dbaileychess
Copy link
Collaborator

Currently these two flags cannot be used at the same time.

My idea is, for a given relationship:

foo.fbs

include "bar/bar.fbs";

a single foo_generated.ts is produced that

  1. Doesn't have any imports (apart from flatbuffers runtime)
  2. exports all types:
export class Bar {}
export class Foo {}
@dbaileychess
Copy link
Collaborator Author

@jkuszmaul Is this the expected out in this case?

@jkuszmaul
Copy link
Contributor

jkuszmaul commented Aug 15, 2022

@dbaileychess Yep. The main ambiguity is (I think) how to manage similarly named objects in different namespaces. I think the current --gen-all without --ts-flat-files handles this by prefixing type names with the fully qualified namespace if needed, but I'm not sure how well-defined that is.

foo.fbs

namespace foo;
include "bar.fbs"

table Foo {
  foo:int (id: 0);
}

bar.fbs

namespace bar;

table Foo {
  bar:string (id: 0);
}

Then you have two objects named Foo, and there's a legitimate question as to what name they should be exported as. I think if we replicated existing logic you'd end up with

foo_generated.ts

export class Foo {
  foo(): number {}
};

export class barFoo {
  bar(): string {}
};

@dbaileychess
Copy link
Collaborator Author

Thanks for the hint about namespacing.

Is barFoo what is preferred? What about bar_Foo or something?

@dbaileychess
Copy link
Collaborator Author

Ok, I am going to worry about the namespace aliasing in another issue, as I don't want to conflate the two.

@jkuszmaul
Copy link
Contributor

I don't have strong feelings about the actual naming style. For consistency with how the non---ts-flat-files codegen works, I don't think you want the underscore. See, e.g., how it is handled with the overloaded Monster name in monster_test.ts:

export { Monster as MyGameExample2Monster, MonsterT as MyGameExample2MonsterT } from './my-game/example2/monster';
export { Ability, AbilityT } from './my-game/example/ability';
export { Any, unionToAny, unionListToAny } from './my-game/example/any';
export { AnyAmbiguousAliases, unionToAnyAmbiguousAliases, unionListToAnyAmbiguousAliases } from './my-game/example/any-ambiguous-aliases';
export { AnyUniqueAliases, unionToAnyUniqueAliases, unionListToAnyUniqueAliases } from './my-game/example/any-unique-aliases';
export { Color } from './my-game/example/color';
export { Monster, MonsterT } from './my-game/example/monster';

The quibble I have with that approach is that it is not obvious to the user which name is likely to end up prefixed and which will show up cleanly.

@jkuszmaul
Copy link
Contributor

And any discussion of how to manage these conflicts should go along with whatever #7448 does.

I'm also realizing that it would also be entirely consistent with the goals of both of the flags to have --gen-all --ts-flat-files generate multiple files, as long as it is still one per input file, which would avoid the whole namespacing issue. Although there is also utility in the "merge them all together into one" option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants