google
[TS] Add reflection library for typescript
This is a potential upstreaming of https://github.com/frc971/971-Robot-Code/blob/master/aos/network/www/reflection.ts -- I'm not sure if it's exactly something we want to incorporate into the flatbuffers repo itself, but I wanted to at least get this code visible for anyone who might want it.
This does a few things:
- Adds a reflection.ts that depends on the --ts-flat-file codegen for reflection.fbs, presuming that the generated code is available in the flatbuffers_reflection package.
- Make bazel rules for the aforementioned reflection.ts, also making it available in the flatbuffers_reflection package.
- Add a rudimentary test (using bazel's nodejs_test rule) for the reflection code.
This only really works great within the Bazel ecosystem (I haven't worked out the imports for more typical setups), but I wanted to get this code cleaned up and figured I'd get at least some visibility to upstream in case we do want to pull it into the main repo.
Note that this also includes #7300, because I've no clue how you're supposed to handle dependent PR's in github these days.
Alternative approach for making this accessible to people (without the weird flatbuffers_reflection package that only even exists if using the bazel rules) is to check in a codegen'd reflection_generated.ts (like we do with C++) and then put it and the new reflection.ts in the main flatbuffers library. That does somewhat offend my sensibilities since it checks in generated code, but would probably be more helpful to people. It's easy to change either way.
@dbaileychess not sure who to ask for review on this, but the code itself is in a state where it can be reviewed. I don't really thing it's current form is exactly what we want in terms of package management for how to provide the reflection library to users, but since I'm not a heavy typescript user myself, I'm looking for feedback on what would make sense before committing to something.
It seems like the current options are to either:
(a) Slightly tweak what I have now so that things work for non-bazel users, and keep a separate flatbuffers_reflection (or similarly named) package that is separate from the regular flatbuffers package, and contains the reflection_generated.ts and reflection.ts. This simplifies some of the code (e.g., don't have to worry about circular dependencies in the codegen), but requires creating a second npm package.
(b) Put reflection_generated.ts and reflection.ts into the flatbuffers package itself. This may require changes to reflection_generated.ts since currently it attempts to import flatbuffers. I could make those changes manually (obnoxious to maintain in the future) or do something more complex in the codegen (also obnoxious to write and maintain).
@jkuszmaul Sorry for the delay, can you fix the merge and I can take a look.
@dbaileychess conflicts resolved. My questions from https://github.com/google/flatbuffers/pull/7301#issuecomment-1163419068 stand, although that doesn't really affect the code itself.
@jkuszmaul Do you mind if I wait on this until I get v2.0.7 out? Hopefully in the next week or so.
@dbaileychess merged latest master (I've been a bit distracted lately--sorry for the slow response). My early comments/questions about package management still apply.
@bjornharrtell Can you respond to the package manage questions above? Basically, how would we package the reflection library of TS.
Converting to draft so I don't keep opening this up to look at the progress :) @bjornharrtell and @jkuszmaul Feel free to reopen/close as you see fit
Converting to draft so I don't keep opening this up to look at the progress :) @bjornharrtell and @jkuszmaul Feel free to reopen/close as you see fit
This seems to be rather bazel specific so I cannot at this point give much feedback. However, it seems it also relies on pre https://github.com/google/flatbuffers/pull/7510 behaviour so should probably revisited after we can get that in.
I haven't had the time to come back to this in a bit. I'll revive it when I next find myself irritated by the fact that I'm maintaining a local diff because of this.
@jkuszmaul I'm just closing it so its not on the list of PRs, feel free to reopen later.