[TS] Codegen should import from individual modules

[bjornharrtell]

This will allow more more optimal and tree-shakable final output also when transpiled to modern JS.

NOTE: Not relevant for current JS target as it's ES5-ish. Might not be worth pursuing a modern JS target as that can be transpiled to from the TS target.

[bjornharrtell]

Started to look at this again now but caught #6286.

I believe we should also avoid generating namespace at all and instead output individual modules as separate files.

I think this will require so many differences from the existing combined JS/TS generator that a new generator class will be needed and the old one can be kept around for those who want old style JS generation. The new generator could be TS generation only and leave JS targets to transpilation.

Do you agree with this @krojew?

[krojew]

What about --gen-all?

[krojew]

I tried looking into this and it seems we have a giant mess after #6094. How do you import flatbuffers into TS now?

[bjornharrtell]

@krojew not sure what you mean. Import should be the same as nothing have changed with regard to @types/flatbuffers, i.e it's assuming flatbuffers namespace exist globally.

That said the situation could be improved in two ways:

• @types/flatbuffers should be obsoleted by having the flatbuffers package contain its own complete typings
• Generated code should import from individual modules (essentially deprecating the flatbuffers namespace)

[krojew]

That's the problem I ran into. I assumed @types will not be needed anymore, since we have native TS. But you can't import * from flatbuffers without them. I think typings should be removed altogether. We can then optimize imports, fix tests and introduce proper long type.

[bjornharrtell]

Fully agreed @krojew but not that easy as I'm not sure we can remove the typings and still rely on a global "flatbuffers" so a large chunk of the work needs to be done in one go.

[krojew]

We don't need to rely on global flatbuffers being present. By default, we add an import statement (which was pretty useless in the old days, which everyone disabled) so we just need a flatbuffers package to be present. But that leads to some problems which must be solved:

1. Local tests need to use local package.
2. We need to avoid using typings.
3. We need to support js somehow.

Now it's a good time to make big breaking changes, since it's likely we'll have a major version bump.

[bjornharrtell]

Ok, so in my opinion it would be good to get rid of these modules exporting namespaces (they exist only to be compatible with current code gen):

• https://github.com/google/flatbuffers/blob/master/ts/flatbuffers.ts
• https://github.com/google/flatbuffers/blob/master/ts/flexbuffers.ts

But that means code gen needs to change for both TS and JS to produce code (TS and ES6-level JS) that imports from individual modules. However I also think that in doing so, the code gen should also produce exported individual modules instead of the namespaced single module thing it does now otherwise all of this would look messy.

If an end user needs ES5 compatible JS it should be up to that end user to transpile it to that target.

[krojew]

I'm a bit concerned about the legacy JS support in such case, although the changes are reasonable. What do you think @aardappel ?

[bjornharrtell]

@krojew we don't need to break compatibility. If we keep the mentioned modules around we can leave JS code gen as is and focus on improving the TS code gen only.

[krojew]

Codegen is one thing, but we need to make drastic changes to the lib itself.

[bjornharrtell]

@krojew I'm not in the loop of the need of any drastic/breaking changes except #6289?

[krojew]

Tried some experiments and looks like #6094 broke some things. Our current FB lib is no longer compatible with @types/flatbuffers and also not compatible with with autogenerated import.

[bjornharrtell]

@krojew: ok that's unfortunate and I was not aware of it. The lib is obviously compatible with JavaScriptTest.js which is what I used as the compatibility target for #6094. I assumed that TypeScript worked because the tests passed at CI.

[krojew]

Can you add a PR which makes TS tests pass with fb import enabled?

[bjornharrtell]

Unsure at this point but haven't had much time yet. As I can't make tests work as is (with no FB import AFAIK) I need to dig deeper.

[jkuszmaul]

Just as a general comment as someone who just started dealing with the Flatbuffer typescript libraries, it would be great to have the imports set up so that we don't need to deal with namespaces on top of modules and adding the actual flatbuffers import to the codegen so that we don't need to deal with @types. Bazel rules would also be nice. I'd be happy to help, but don't know how strong an interface needs to be maintained.

[krojew]

@jkuszmaul this would be great to have. @bjornharrtell what do you think about not fixing current implementation to work with or without built-in imports and simply switching to a modularized version?

[bjornharrtell]

@krojew agreed. And to me it then makes sense to treat JS the same. Or possibly only support TS codegen with JS through compilation of TS only.

[krojew]

@bjornharrtell can you look into this?

[bjornharrtell]

@krojew I'm willing to but unsure when I get enough time for it. For me this will require a substantial effort to get to completion.

[aardappel]

@krojew hard for me to tell how many people will still need older JS. If they do, how easy is it to transpile, and how hard is it to provide a native version for us? I know internally at Google we have people using JS with the Closure compiler, and I wonder if all this stuff will break them, and how.

[krojew]

Transpilation is pretty easy - one just needs to specify desired JS flavor and tell TS to transpile it. In this sense, we will support all possible JS versions, including ones working right now.

[bjornharrtell]

@krojew and @aardappel I think we should be more clear about what we aim for with regard to code gen, npm package and what's possible for flatbuffers 1.x vs 2.x.

For the npm package I believe it is possible to make it offer two variants for maximum compatibility:

1. We can keep the transpiled JS single module with global flatbuffers/flexbuffers namespace to remain compatible with 1.x
2. We can add ES6-level JS modules compiled from TS with bundled typings

I believe we might already be there but needs verification and there is the issue with the test failures.

For the code gen I see three variants possible:

1. Keep the current JS code gen as is, which will then depend on 1 in the npm package
2. Offer ES6-level JS modules code gen which will depend on 2 in the npm package
3. Offer TS modules code gen which will depend on 2 in the npm package

To reduce amount of work I would suggest dropping code gen 2 above because it does not have much value. Users that want that target should be able to transpile 3 to 2 on their own.

If there is to be a breaking flatbuffers 2.x release in the future I would also consider dropping compatibility (the 1:s above) to further reduce work and maintenance. I do not know closure compiler setups can deal with this but it doesn't seem sensible to let that dictate a 2.x to force many years of maintenance of ES5 level JavaScript support?

[krojew]

My opinion is we should drop package numer 1 right now and go straight to modules. As far as codegen is involved, I believe we should keep options 2 and 3 to allow users to choose between JS and TS. After all, if we publish a transpiled package with typings, there's no reason not to support both JS and TS generation, especially given the similarities.

[bjornharrtell]

@krojew ok agreed but I would implement it so that code gen internally is only TS and depend on tsc to compile to JS target.

@aardappel do you agree? I think it's reasonable to expect that any living project that depends on JS should be able to handle ES6-level code and modules.

[krojew]

I think that's just adding more work. The existing codegen can only use different imports and remove flatbuffers. prefix to make it work with new lib, as far as I understand.

[bjornharrtell]

@krojew hmm yes, I think you are right. For whatever reason I had gotten into my mind that I needed a fresh start on the code gen, perhaps because I found it too messy to make the flatbuffers. removal optional but if we instead just remove that the task now feel smallish even. (though still a bit tricky to track and import only the used symbols)

[bjornharrtell]

@krojew now I remember an additional reason why I thought TS/JS code gen need a fresh start for modules... current code gen generates old style namespaced output into a single script file when it really should be separate files/modules to be properly modernized. Thoughts?

[krojew]

We can remove namespace from TS output, but that would be incompatible with the --gen-all flag. So I see the following approaches:

1. Keep namespaces as are. Kind of ugly, but works.
2. Remove namespaces when generating single files and keep --gen-all as is.
3. Don't support --gen-all.

To be honest, I'm all for the last option.

[bjornharrtell]

@krojew mabye I'm misunderstanding something here but current code gen for JS/TS outputs a single file regardless of --gen-all and we can't just remove namespaces, they need to be replaced by either ugly symbol prefixes or break out into individual files in folders that represent the namespace as module paths.

[krojew]

No, it outputs a file per .fbs. It only outputs single files with --gen-all.

[bjornharrtell]

@krojew I meant it does not split namespaces/tables into separate modules similar to other languages that have support for that. And unless you do that you will have to simulate namespace in some way inside a single module/file...

[bjornharrtell]

I've experimented with a spike at #6302 that doesn't change anything else than dropping the prefix (adding imports not automated yet) and it seems possible to make it work but I still feel quite strongly that if we are going after modern modularized TS/JS in general the code gen should reflect that and also create similar modularized code.

[krojew]

Ok, I get what you mean. We can do the same thing java generator does and split them. As for lib imports - we need to alias them (or namespace in another way), because the user can still create tables named Builder etc.

[aardappel]

Again, I am not familiar with what's common in this eco-system. I will defer to what you guys think is reasonable.

[krojew]

I'm slowly starting to lean towards dropping direct codegen for JS, and rely on transpilation. This would include an additional step for our users, but it's less maintenance for us, which is more or less a copy of TS. Ideally, it would be grat if flatc would detect tsc and do the transpilation in flight. What do you think @bjornharrtell @aardappel ?

[bjornharrtell]

@krojew I agree it should make the codegen a bit more simple and as we have concluded the codegen needs a bit of work. I agree that if flatc could just detect and use tsc it would be nice, but as JavaScript have so many flavors that you might want to tweak (ES5, ES6 etc..) perhaps it's best to leave it up to the user.

[krojew]

That's one of the arguments that's pushing me towards eliminating direct JS support - given current zoo of JS flavors, it might be a good idea to have the user decide. We can try to make flatc help with this somehow in the future.

[krojew]

@aardappel do you think such approach is acceptable? It would simplify generation and testing a lot.

[aardappel]

Hmm, not sure if I like the idea of flatc running other tools.. generally think this is best to be explicit. Or there could be a shell script/batch file that invokes both.

Then again, if it was explict with a flag, i.e. --run-tsc --js-es5 or whatever, then maybe its ok?

Coincidentally, I recently came across https://github.com/sheredom/subprocess.h which is something we would need, to portably run a tool on all OSes. Since currently FlatBuffers has no "dependencies" such a single header lib could be nice to use.

[krojew]

We can leave it to the user. We should also update the documentation then with a JS tutorial involving tsc.

[krojew]

This most definitely should be linked to #6353

[aardappel]

Since this issue is linked to 2.0 release, how are we doing on it?

[krojew]

@bjornharrtell this can be closed with the new TS generation, right?

[bjornharrtell]

@krojew yep!