[JS/TS] Modernize TypeScript / JavaScript flatbuffers support

[bjornharrtell]

Contains an initial attempt to convert existing base JavaScript to TypeScript.

Ref #6094.

TODOs

• Code gen option to import individual modules for TS (and possibly ES-modules compatible JS) (worth a separate PR)
• Test cases
• Transpile to compatible JS target

[aardappel]

I have no particular opinion on whether the library should be written in JS or TS (and then translated/adopted to the other), but I do know that we don't want be maintaining two copies of essentially the same code in the repo for very long. So if this is to be merged, transpiling would have to be part of this PR or a follow-up PR.

Also do note we are currently still compatible with all sorts of older JS environments and Google Closure etc, and this transpiling would have to be compatible with that.

@krojew can decide on this :)

[krojew]

Rewriting the JS api to TS was something I've been wanting to do for a long time, but other projects ate all my free time, so it never happened. Thanks for making it happen.

[krojew]

Could you add JS generation? This would serve both as a compatibility layer and we could use existing test cases for smoke tests.

[bjornharrtell]

Thanks @aardappel and @krojew for confirming this is worthwhile. 🙂

I hope to be able to spend some more time on this soon and agree, transpilation is probably a must have and target should remain compatible. I know how to target IE11 but less certain about closure compiler.

[bjornharrtell]

@krojew already have some success. With the commits added I can now successfully transpile (a ./node_modules/.bin/tsc on a clone will overwrite the existing flatbuffers.js with the transpiled result) and the tests run with ./JavaScriptTest.sh pass.

[bjornharrtell]

@krojew / @aardappel I need guidance on how to proceed, specifically:

1. Does it make sense to do the TS code gen in a separate PR? (if so this PR is feature complete)
2. Does the transpiled code work with your requirements for closure compiler (or other requirements that I might not be aware of)?

[aardappel]

I'd say do it in this PR, so we can see the diff between the existing .js and the generated one.

I have no experience programming JS, so I have absolutely no idea what is required for the Closure compiler, or any of the other interesting language version / environment compatibilities we've had to deal with in the past (modules comes to mind). All I know is that we have people relying on this stuff, so this functionality will have to be fixed/reverted if it breaks. To me easiest to see that it won't is to see the diff between the two versions, that preferably is empty :)

What might be an idea is that if the diff between the two versions is obscured by lots of formatting differences, to first land a PR that changes the formatting in the .js version such that as little as possible will change in this PR.

[bjornharrtell]

I'm not certain it will be possible to the the transpilation to produce close to identical output but I will investigate.

As for code generation, I'm proposing the flatc --ts option should import individual modules directly from the TypeScript implementation of the library instead of JavaScript but that this work can be done separate from this PR.

[bjornharrtell]

As far as I've been able to find out it will not be possible to create identical or close to identical output and perhaps more importantly it seems more or less hopeless to ensure closure compiler compatible output.

However I think it would be quite sad if closure compiler will "forever" hinder a more modern JavaScript implementation (especially modules in my opinion) which it would get via TypeScript.

Note the transpiled output appears API compatible and can target a specific browser level compatibility. I've commited the transpiled flatbuffers.js into this PR to demonstrate with dc91eb1.

What are your thoughts @krojew and @aardappel?

[aardappel]

Yeah, not sure what to do about this.. "breaking people" or "maintaining two copies" are both pretty bad outcomes, compared to "the TS runtime is untyped JS" which we have now.

Worse, we use a mono repo internally, meaning that if I import the latest FlatBuffers and it would break people's JS tests because of lack of Closure compatibility, I can't land it at all, affecting all FlatBuffers languages. I'd have to revert this PR and any followups affecting it to make that happen.

That's of course a problem with FlatBuffers being a single repo by itself, but that also has many advantages.

Seems to me generating Closure compatible output would be relatively easy for TS, since it is also some type decls in comments, right?

[krojew]

The attached JS file did compile in https://closure-compiler.appspot.com/home, so I guess it's ok? Maybe someone with more experience in this regard can verify. Nevertheless, I suggest removing the resulting js file from the repo and generating it on the fly when building a npm package.

[bjornharrtell]

@krojew that seems promising. It was a long time ago since I worked with closure compiler. If someone relies on jsdoc type information to be closure compliant it might be a problem.

I agree, the transpiled result should not be in repo.

[krojew]

From my perspective, if we get rid of transpiled js and add a script which builds it, I'm good with this PR. Somebody working with closure should maybe take a look if everything is ok.

[bjornharrtell]

@krojew great, I think I've accomplished that now and also added both a mjs variant and ts to the package so that those who consume the package can pick and choose.

But I'm also looking at the code gen for ts now, it looks like it's not very hard to adjust so that it will import from ts instead of js.

[krojew]

When dealing with codegen, remember to keep backwards compatibility as much as possible.

[bjornharrtell]

@krojew I can make sure code gen does not change existing output but does that also apply to TypeScript code gen? It seems more sensible to import from individual modules. But I can make it opt in.

I realized I have to break apart the implementation to individual modules to really benefit from TypeScript. This is now done and the transpiled result is essentially the same, except you now also get individual modules in addition to the all in one typescript.js and I also generate type definitions.

I tried using this, importing individual modules into my own TypeScript code, and one direct benefit is significantly smaller resulting bundled code.

[bjornharrtell]

@krojew this is now green and ready for proper review. Code gen improvements can and probably should come in a separate PR.

[krojew]

Can you convert TS file names to use lowercase and dashes?

[bjornharrtell]

@krojew done.

[bjornharrtell]

@krojew hang on, #5973 was probably merged after this work so I need to incorporate that.

[bjornharrtell]

@krojew never mind, I see now that flexbuffers is a completely separate implementation. I'm willing to port that too but can be done in a separate PR.

[bjornharrtell]

Rebased and squashed on master.

[aardappel]

so if someone wants to work with JS directly from this repo, they need to have TS installed as well? what are the implications of removing this file?

[bjornharrtell]

@aardappel yes TS will be required for development and flatbuffers.js can be regarded a build artifact, this is why I added typescript as devDependency in package.json. To get flatbuffers.js you would either need to build locally or use the npm package which will contain the build artifacts.

[krojew]

@aardappel anything blocking this?

[aardappel]

Like I said above, I find it impossible to judge who is going to be affected by this..

But lets merge it, and see who comes complaining.. if so, we can figure out how to fix it :)

Thanks @bjornharrtell !

[krojew]

Turns out the old js file is still present. Wasn't the plan to remove it and see who complains? @bjornharrtell @aardappel

[bjornharrtell]

@krojew I don't see it? https://github.com/google/flatbuffers/blob/v1.12.0/js/flatbuffers.js -> poof gone https://github.com/google/flatbuffers/blob/master/js/flatbuffers.js ? :) Sure you are not confusing it with the flexbuffers.js file? That one is still there but is being worked on at #6148.

[krojew]

Ok, maybe I was looking at some specific point in the past and didn't notice.