flatbuffers
flatbuffers copied to clipboard
google
[TS/JS] Entry point per namespace and reworked 1.x compatible single file build
Implements reworked entry point generation, so that each namespace gets an entrypoint and re-exports children. The top level entry point can be bundled up into a single file that should be 1.x compatible. This also has the benefit of avoiding name clashes which occured in the IMHO not so well thought out previous attempts (including my own) to export everything in a single module at root level. A new option is added --ts-entry-points to enable the entry point generation which also implicitly also enables --gen-all because it needs all symbols for the entry points.
Note that this PR also rolls back the change to the npm package entry point name. It was changed to js/index.js some time ago but is now back to js/flatbuffers.js which it was before (also in earlier 2.x versions) because IMHO that was an unnesseary breaking change. Specifically this makes it possible to run the old test suite from flatbuffers 1.x which is added back as tests/ts/JavaScriptTestv1.cjs and it works with no other modification than moving from flatbuffers.Long to native BigInt support which was was a breaking change we made years ago. One could argue we should have kept flatbuffers.Long for backwards compatibility but that ship has sailed and probably not worth the effort anyway IMHO.
The previous implementation of option --ts-flat-files has been replaced by esbuild automation. It simply automates runs of esbuild to produce old style 1.x generation single file bundles that assume flatbuffers is available in global scope.
This PR also removes some left over older generated TS/JS files missed in https://github.com/google/flatbuffers/pull/7508.
References https://github.com/google/flatbuffers/issues/7448.
TODO
- [x] Handle reserved words (currently not replaced on re-export)
- [ ] Check for esbuild availability when
--ts-flat-filesis used - [ ] Fix/update flatc_ts_tests.py
- [ ] Handle case with root namespace table with same name as fbs
I've commited a POC of bundled unminified root level entry point for monster_test.fbs at https://github.com/bjornharrtell/flatbuffers/blob/f046ab497d1030869ecb921607046fee9297ff99/tests/ts/monster_test_generated.js. I've not tested it but it looks ok. It should work with a globally imported flatbuffers and will define a global object monster_test which has the same structure as the flatc from 1.x generated single js file which means that for example monster_test.MyGame.Monster exists as a symbol.
It was generated using esbuild, with the following command line:
esbuild monster_test.ts --bundle --outfile=monster_test_generated.js --global-name=monster_test --external:flatbuffers
Note that by removing --external:flatbuffers flatbuffers is included in the bundle to create a fully stand alone file.
@dbaileychess this is essentially what https://github.com/google/flatbuffers/issues/7485 intends to resolve.
To verify this is actually 1.x compatible I've added the test suite from latest flatbuffers 1.x version in https://github.com/google/flatbuffers/pull/7510/commits/7c96cab51f029c33b4ab0950237e9b577a76f66e and modified to handle the change to tests/ts path and that flatbuffers 2.x no longer supports the custom Long type and instead relies on native BigInt. And against the single file built monster_test_generate.js it passes for me locally (ran it with node JavaScriptTest1.js ./monster_test_generated.js).
I've pushed through with this and moved it out of draft for review. It produces valid single file output that is 1.x compatible for the schemas in tests/ts. However, the flatc ts tests failures remains unresolved. I remain convinced what this PR does is worthwile goal and less hacky/quirky than what it replaces so that we only have two kinds of outputs instead of four (!). What I mean with this is that counting 1.x and 2.x current possible JavaScript/TypeScript output you can get 1.x compat single file builds, my initial attempted "flat" namespace entry points, your (@dbaileychess) initial --ts-flat-files and the "modern" individual modular output introduced with the TS rewrite. With this PR we can go back provide only 1.x compatible single file builds and the individual modular output which is what I see as the big win here.
While I do agree with the general idea of the flatc tests and what their purpose is I remain unconvinced the flatc ts tests provide real value because of the contrived schemas and that even the current implementation in master does not produce something that is valid JavaScript or TypeScript from such schemas.
@dbaileychess I sincerely hope we can discuss or move forward with this in some way. I've put many many hours of effort into this PR.
I'm approving this. Please update the conflicts and I can prioritize.
I will take responsibility of fixing internal Google users of this. My plan is to release a version before I merge this in, merge that into Google, and then release this in its own version, so I can miminize the scope of changes. Sounds like a plan?
Thanks that is great news to me @dbaileychess and I'll try to assist if you get stuck. I'll get this rebased ASAP.
Rebased but new tests for arrays_test_complex schema fails due to what seems that my implementation doesn't properly consider "prefix" (-o parameter). I'll need a bit more time to fix that, hopefully sooner than later.
I believe I've have the -o param / outputpath handling fixed up now.
One detail to note is that the single file bundles created when using --ts-flat-files is JavaScript - no TypeScript equivalent are produced. I don't see a use case for a TypeScript bundle and don't know of any sensible means of creating it.
I noted one remaining known issue with the generated bundles which is that the object API is not available (not exported). Not sure how important this is at this point, but should not be too hard to fix. @dbaileychess do you feel it's blocking this or that we can track it as a separate issue?
I also noted an issue with array_test_complex but is AFAIK unrelated to this PR (faulty --gen-mutable code, just disabled it for now). I can track it separately later.
I note an issue with generate_code.py. It tried to use prefix to handle the tests/ts subfolder but that is no good, it changes the relative paths for the generated code. I tried to fix that by using the cwd param for flatc() function but it doesn't work it doesn't find the schema.
buildkite issue looks like something is assuming typescript bundle output which as mentioned is no longer a thing. Bundled output is javascript only. Unable to quickly figure out where that assumption is...
I've attempted to fix bazel but I'm stuck with that it needs esbuild installed to generate the bundled code but I can't add it in presubmit.yml because it doesn't seem node is available at all. I'm not sure where to else try and add it. Any ideas @dbaileychess ?
Looks like it needs to be plugged in with https://www.npmjs.com/package/@bazel/esbuild... Looks like bazel has it's own entire ecosystem and wrapped tools.
@jkuszmaul Could you help with this PR? The buildkite CI is not working since the esbuild binary isn't present for bazel. I tried adding the dependency, but I have never use bazel/yarn/or really npm, so I am not sure what needs to be done.
@dbaileychess I can help make what you want to build build; it should be reasonably straightforwards for me to just do. It sounds like you are still deciding exactly where to call esbuild, so I don't want to make it work with the solution you aren't going to use.
@jkuszmaul Thanks for your fast reply and willingness!
Do you have thoughts on how such a bundler should be called? I know you did a lot with --ts-flat-files that this is replacing, so you might have some opinions in this space.
@jkuszmaul I've just now reworked so that it will not run esbuild automatically (but it will output it as a hint). I will try to get it into the testing scripts instead and actually verify the output. Let me know if you have any questions on this PR in general as we try to make it mergable.
I've now finished implementation so that bundled commonjs modules are output by TypeScriptTest.py and tests from flatbuffers 1.x (JavaScriptTestv1.cjs) are run against /monster_test_generated.cjs to verify both that the bundle works and is 1.x compatible.
@jkuszmaul for some reason I can't reply on your comment above about the print out. Indeed the print out is the only thing that --ts-flat-files does now and I agree it would be good to document somewhere. But it's not trivial to do meaningful documentation on the subject.
Oh, doesn't this mean we lose type information when building to a single file? Is there a way for esbuild to generate a declaration file or something so that we can still rely on type information? Or am I missing something?
https://github.com/jkuszmaul/flatbuffers/tree/ns-entry-points gets most of the way there on the bazel rules, but I stopped when I ran into the wrong path getting printed from flatc, and since some discussions are still outstanding.
Oh, doesn't this mean we lose type information when building to a single file? Is there a way for esbuild to generate a declaration file or something so that we can still rely on type information? Or am I missing something?
Not sure what you mean exactly. When consuming from typescript you would typically use modules directly not this single file bundle. The single file bundle is legacy JavaScript compatibility in my view.
You can get a source map though with esbuild with flag --sourcemap perhaps that is what you are after here?
Not sure what you mean exactly. When consuming from typescript you would typically use modules directly not this single file bundle. The single file bundle is legacy JavaScript compatibility in my view.
You can get a source map though with esbuild with flag --sourcemap perhaps that is what you are after here?
The goal of the --ts-flat-files flag is specifically to produce a single typescript file. The reason for this is that Bazel really wants to statically know exactly what files are going to be output by a build step prior to reading the input file, but we still want the benefits of typescript. A similar objective would be achieved by producing a .d.ts file and implementing .js/.cjs file.
Not sure what you mean exactly. When consuming from typescript you would typically use modules directly not this single file bundle. The single file bundle is legacy JavaScript compatibility in my view. You can get a source map though with esbuild with flag --sourcemap perhaps that is what you are after here?
The goal of the
--ts-flat-filesflag is specifically to produce a single typescript file. The reason for this is that Bazel really wants to statically know exactly what files are going to be output by a build step prior to reading the input file, but we still want the benefits of typescript. A similar objective would be achieved by producing a .d.ts file and implementing .js/.cjs file.
This PR makes it so that the modular output can be used to bundle JavaScript in a standard way. I'm not aware of any standard tools for TypeScript bundling. I know that is contrary to what the flag says and I propose above that the name should be changed. In my understanding --ts-flat-files was a hack to be able to produce a single JavaScript bundle to resemble what you got with flatbuffers 1.0, not a goal itself.
There seems to be some work towards creating bundles type definitions using esbuild, rollup and other tools but I see nothing official at his point. But the whole thing of Bazel driving this need feels wrong to me. :(
There seems to be some work towards creating bundles type definitions using esbuild, rollup and other tools but I see nothing official at his point. But the whole thing of Bazel driving this need feels wrong to me. :(
It definitely feels awkward, but it's a design decision that Bazel made, and my general experience has been that it's not typically too hard to accommodate. The main alternative in this case is to require that users list out all the files that will be generated for each fbs file (or have them list every type and then generate the list of files to expect). But that sucks for users, hence why I preferred to just concatenate the files in flatc.
There seems to be some work towards creating bundles type definitions using esbuild, rollup and other tools but I see nothing official at his point. But the whole thing of Bazel driving this need feels wrong to me. :(
It definitely feels awkward, but it's a design decision that Bazel made, and my general experience has been that it's not typically too hard to accommodate. The main alternative in this case is to require that users list out all the files that will be generated for each fbs file (or have them list every type and then generate the list of files to expect). But that sucks for users, hence why I preferred to just concatenate the files in flatc.
I would get sad to see this PR go stale so I'm willing to try and see if it's possible to generate the type declaration bundle of you think that it a way forward.
There seems to be some work towards creating bundles type definitions using esbuild, rollup and other tools but I see nothing official at his point. But the whole thing of Bazel driving this need feels wrong to me. :(
It definitely feels awkward, but it's a design decision that Bazel made, and my general experience has been that it's not typically too hard to accommodate. The main alternative in this case is to require that users list out all the files that will be generated for each fbs file (or have them list every type and then generate the list of files to expect). But that sucks for users, hence why I preferred to just concatenate the files in flatc.
I would get sad to see this PR go stale so I'm willing to try and see if it's possible to generate the type declaration bundle of you think that it a way forward.
This PR definitely feels like the right direction to be going for generating single files (i.e., relying on auto-bundlers instead of trying to manually concatenate files and adopt the maintenance burden of keeping that correct). If there really are no good options for generating the type declarations, I think I'd be willing to take your current approach, but it would definitely feel like an unfortunate step backwards.
@jkuszmaul I've spent some time trying out different experimental ways to bundle type declarations without success. The closed issue at https://github.com/Microsoft/TypeScript/issues/4433 is also telling that the interest to provide the ability to bundle type declaration is low. In my opinion if you want the benefits of TypeScript you really need to use the modular output as is.
I've added generation of the modular type declaration to tests/ts and also added the generated output to this PR.
@jkuszmaul and @dbaileychess I hope this can progress regardless. Let me know if there is something else I can do.
Unfortunately I find it hard to resolve the merge with master now due to https://github.com/google/flatbuffers/pull/7748. Will make a new attempt when I have some more time.
I've been able to do the merge now as proper as I can but tests specific to no_import_ext fail like follows:
no_import_ext/optional-scalars.ts:3:30 - error TS2835: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean './optional-scalars/optional-byte.js'?
This is expected to me and I don't quite understand how the tests passed in #7748. @ink-su and @sunwen18 do you have any ideas?
Nevermind I think I understand it now, it needs to be tested using the "old" node resolution mode of typescript. I've separated the tests.
But I see another issue now. When using prefix (-o) it looks like module paths become wrong for no_import_ext but strangely enough not for union_vector... sigh.