TypeScript support #4232

krojew · 2017-03-17T10:08:46Z

Thank you for submitting a PR!

Please make sure you include the names of the affected language(s) in your PR title.
This helps us get the correct maintainers to look at your issue.

If you make changes to any of the code generators, be sure to run
cd tests && sh generate_code.sh (or equivalent .bat) and include the generated
code changes in the PR. This allows us to better see the effect of the PR.

If your PR includes C++ code, please adhere to the Google C++ Style Guide,
and don't forget we try to support older compilers (e.g. VS2010, GCC 4.6.3),
so only some C++11 support is available.

Include other details as appropriate.

Thanks!

googlebot · 2017-03-17T10:08:50Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

krojew · 2017-03-17T10:10:49Z

I signed it!

aardappel · 2017-03-18T16:52:43Z

Thanks, this seems like it would be a useful addition!

Since TypeScript mostly overlaps with JavaScript, this would be better an an option to the existing JavaScript generator (in IDLOptions), rather than a copy of the existing generator (since now we have have to maintain 2 of them).

Also please add some description to your PR?

krojew · 2017-03-18T17:03:53Z

I based this PR on previous work in #4065 where TS and JS were merged in one file. This resulted in a lot of if (ts) statements, not to mention some other problems (exports for js were unneeded for ts; missing imports for ts, while they're unneeded for js; missing namespaced external dependencies for ts, etc.). That's why I decided to separate those two.

googlebot · 2017-03-18T17:03:56Z

CLAs look good, thanks!

aardappel · 2017-03-18T17:16:55Z

Well, yes, you'd have to factor the code such to keep the amount of if-thens at a minimum. But sharing the code still seems very useful to me.

krojew · 2017-03-18T17:30:34Z

Ok, so should I merge them?

aardappel · 2017-03-20T22:27:31Z

Yes, please do.

fasterthanlime · 2017-03-24T16:51:43Z

I just discovered this PR, when I was cleaning up #4065 myself!

Since you got further than me, I submitted mine anyway for reference (you'll see I made some slightly different choices), I'm explaning my rationale here: #4244

fasterthanlime

I've left a few comments regarding some style issues / opportunities for deduplication, overall I'm really excited about this PR landing :)

fasterthanlime · 2017-03-24T16:53:49Z

src/idl_gen_js.cpp

    if (IsEverythingGenerated()) return true;

-    std::string enum_code, struct_code, exports_code, code;
+    ImportedFileSet importedFiles;


as per the google C++ styleguide, importedFiles => imported_files

fasterthanlime · 2017-03-24T16:55:33Z

src/idl_gen_js.cpp

-      : BaseGenerator(parser, path, file_name, "", "."){};
+              const std::string &file_name, bool ts)
+      : BaseGenerator(parser, path, file_name, "", ".")
+      , ts_(ts)


Instead of passing a ts boolean, I recommend using the same approach as idl_gen_general and use an array of structs: here's one way to do it

fasterthanlime · 2017-03-24T16:56:20Z

src/idl_gen_js.cpp

-  return path + file_name + "_generated.js";
+                                     const std::string &file_name,
+                                     bool ts) {
+  return path + file_name + ((ts) ? ("_generated.ts") : ("_generated.js"));


If you opt for the JsLangParams array of structs, it's easier to have the third parameter be a const JsLangParams & and use lang.file_extension here

fasterthanlime · 2017-03-24T16:57:31Z

src/idl_gen_js.cpp


    // Output the main declaration code from above.
+    if (ts_) {
+        code += import_code;


for non-typescript, wouldn't import_code be the empty string anyway? it seems like the surrounding if could be removed, making the += unconditional

Yes, it would be ignored, but then there's no point in generating it in the first place. As the old saying goes - some work is more work than no work :)

I'm pretty sure appending an empty string in C++ is very close to "no work" :) Besides it's not in a loop or anything (happens at most once per module) so it's not performance critical (not that flatc is in the first place). I still suggest removing the condition to remove "branching on type" (see anti-if campaign).

That way, if a third javascript-like language is added, the condition doesn't have to change, import_code will contain what's needed!

fasterthanlime · 2017-03-24T16:58:17Z

src/idl_gen_js.cpp

    code += struct_code;

-    if (!exports_code.empty() && !parser_.opts.skip_js_exports) {
+    if (!ts_ && !exports_code.empty() && !parser_.opts.skip_js_exports) {


This seems like a good candidate for a boolean in the JsLangParameters struct (explicit_exports for example, true for js, false for ts)

fasterthanlime · 2017-03-24T16:59:38Z

src/idl_gen_js.cpp

+      const auto basename =
+          flatbuffers::StripPath(flatbuffers::StripExtension(file));
+      if (basename != file_name_) {
+        code += "import * as "+ GenFileNamespacePrefix(file) + " from \"./" + basename + "_generated\";\n";


since it's used here again, _generated has become a magic string - if changed in one place it'll break in the other, so it's a good candidate for a constant instead

fasterthanlime · 2017-03-24T16:59:59Z

src/idl_gen_js.cpp

  void generateStructs(std::string *decl_code_ptr,
-                       std::string *exports_code_ptr) {
+                       std::string *exports_code_ptr,
+                       ImportedFileSet &importedFiles) {


importedFiles => imported_files

fasterthanlime · 2017-03-24T17:02:43Z

src/idl_gen_js.cpp

-        exports += "this." + *it + " = " + *it + ";\n";
+    if (ts_) {
+      if (it->find('.') == std::string::npos) {
+        code += "import { flatbuffers } from \"./flatbuffers\"\n";


the original PR also has this import emit in a loop, but I wasn't able to figure out why: it seems to be that it only needs to be emitted once?

also, to make the code self-documenting, it might be worth it to assign it->find('.') == std::string::npos to a local bool variable that indicates what that condition is in fact testing

speaking of flatbuffers.ts, I don't see it anywhere in the PR, is that normal? am I missing something? is it also supposed to be generated by flatc?

The issue here is that flatbuffers has a .js import but no .d.ts file. I have one locally, and there are more floating around on the web, but I don't know if I should include it in this PR. Probably so. Nevertheless, that's why the flatbuffers import is optional.

I see. I suppose either shipping flatbuffers.ts, or a flatbuffers.d.ts is acceptable. The latter is probably superior since it would avoid code duplication between the javascript & the typescript versions?

Yeah, having one of them is really a must. .d.ts seems more practical as you said. If nobody has any objections, I'll add the one I'm using.

On the other hand, it would be better to submit it to Definitely Typed.

DefinitelyTyped

or just a @types/flatbuffers package on npm, see https://www.npmjs.com/~types

fasterthanlime · 2017-03-24T17:07:13Z

src/idl_gen_js.cpp

+                       const std::string &file_name) {
+  return MakeRule(parser, path, file_name, true);
+}
+


With JsLangParameters, these can be deduplicated, here's one way it can be done.

krojew · 2017-03-24T17:57:14Z

Thanks for all your comments. I'll add more changes as soon as possible.

fasterthanlime · 2017-03-27T08:39:12Z

@aardappel would it be possible for you to take a look at this PR again? Still a bit if-happy but I believe it addresses most of the points in the original review of #4065

aardappel · 2017-03-27T20:11:23Z

src/idl_gen_js.cpp

      *annotations += "} " + nameprefix + field.name + "\n";
-      *arguments += ", " + nameprefix + field.name;
+
+      if (lang_.language == IDLOptions::kTs) {


this if-then can be simplified I think :)

aardappel · 2017-03-27T20:14:45Z

This PR looks pretty reasonable to me now. It appears to already have absorbed some changes from #4244. @fasterthanlime are you happy with the state of this PR?

fasterthanlime · 2017-03-28T08:13:02Z

This PR looks pretty reasonable to me now. It appears to already have absorbed some changes from #4244. @fasterthanlime are you happy with the state of this PR?

Yep, I reviewed the code and @krojew was reactive in incorporating that feedback into the PR!

Ifs apart, maybe the last remaining point is testing - it'd be nice to at least check that the generated .ts validates with the typescript compiler, but that would require a full-blown node.js runtime on Travis & AppVeyor - does the current CI setup already support that?

krojew · 2017-03-28T08:41:19Z

I'm actually using generated TS files for my project (almost 100 .ts message files, with lots of includes/imports) and it works fine. TS is used with noImplicitThis, noUnusedParameters, noUnusedLocals, noImplicitAny and noImplicitReturns.

krojew · 2017-03-28T16:57:26Z

TS typings just landed in Definitely Typed (so they will be available in @types soon).

krojew · 2017-04-06T12:38:15Z

You're right - Long is problematic. In my project, I solved it by simply not using numbers, where Long is expected, and passing Longs all along. And there's this hack:

export function numberToLong(value: Number): flatbuffers.Long {
    let padded = value.toString(2).padStart(64, '0');
    return flatbuffers.Long.create(parseInt(padded.substring(32), 2), parseInt(padded.substring(0, 32), 2));
}

Nevertheless, this is a topic for another PR.

aardappel · 2017-04-07T16:44:20Z

@krojew The output from translating TS code shouldn't overwrite the JS code, and it shouldn't be in this commit. We want to be able to track changes to JS code seperately.

krojew · 2017-04-07T18:48:56Z

ok, done

aardappel · 2017-04-07T21:18:42Z

Thanks! I can merge, if there are no further comments.

aardappel · 2017-04-10T17:15:06Z

A couple of issues when running this locally.

when you run sh generate_code.sh it produces changed .ts files. This is probably because it is being run without --no-fb-import. We need to be able to run this script with no new changes.
Your test script fails for me. I get permission denied on the first npm command. It would really be better if this test can run locally without having install and de-install npm packages, and just use the local files.
Renaming and overwriting the existing JS code is also very fragile. Maybe better to rename the .ts file so it doesn't have the same base name as the .js code.

krojew · 2017-04-10T17:37:36Z

Unfortunately, you need to be able to run npm since the test needs flatbuffers typings (and the latest version, which doesn't seem to be merged yet). Otherwise the compilation will fail.
The js file is backed up and restored by the script, so overwriting shouldn't be a problem. It was the fastest way to utilize existing JS test suite.

aardappel · 2017-04-10T23:59:07Z

Why? the JS test runs locally, and it be great to do that for TS too.
Still, it needs to output the same code as generate_code.sh does. And renaming of the .ts is better than overwriting the .js.

krojew · 2017-04-11T06:01:50Z

I can refactor the JS and TS tests to be more independent. Note that not using npm requires us to include flatbuffers typings in the source and manually update them every time those change. Is that really what we want?

aardappel · 2017-04-12T16:34:24Z

Not being a JS/TS user, I guess I don't understand the requirements. Doesn't TS already have types in the source? Why do they need to be external, and why do they need to be retrieved through npm?

krojew · 2017-04-12T16:42:22Z

When dealing with external JS libraries, TS doesn't know anything about what's inside. That's why it needs a helper declaration file, usually with a .d.ts extension, which tells the TS compiler what is really contained in a given JS lib. For flatbuffers, I've made such declaration, and it's available here: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/flatbuffers/index.d.ts
Retrieving those declarations is done via npm. Without them, the compiler will just throw an error each time it sees flatbuffers usage.

aardappel · 2017-04-12T18:42:37Z

So why don't we also have that file in this git repo? If people make changes to the TS implementation, which presumably also changes the .d.ts, they need to simultaneously do a PR here and update files elsewhere? Wouldn't it be more convenient if that was all in one place?

krojew · 2017-04-12T19:21:09Z

The standard place for typings is the @types repository which fetches files from DefinitelyTyped. Storing one locally here would mean synchronizing it with DT, so other people could get it the standard way. I think it's more practical to update it once in DT and make the test fetch it from @types. That's the typical workflow for TS projects.

aardappel · 2017-04-13T00:53:29Z

So to make changes to TS, you have to make a PR to 2 repositories? Seems inconvenient to me, but if that is standard.. I guess so.

Either way, maybe you can address my other 2 suggestions above (the non-npm ones).

krojew · 2017-04-13T06:06:16Z

It might be inconvenient to library authors, but it's quite convenient to the users - all that's needed to use flatbuffers in TS is: npm install flatbuffers @types/flatbuffers
As for the other points - I'll make another PR.

liukun · 2017-04-14T16:28:30Z

@krojew Thanks for you work!
However, IMHO @types is mainly for js libraries whose origin repo not easy to modify. A ts library typically includes type definitions itself. For example, if you try npm install @types/moment, you will get

npm WARN deprecated @types/moment@2.13.0: This is a stub types definition for Moment (https://github.com/moment/moment). Moment provides its own type definitions, so you don't need @types/moment installed!

aardappel · 2017-04-17T18:12:09Z

@liukun thanks.. I was wondering that too.

krojew · 2017-04-18T06:14:10Z

@liukun most of JS libs I've seen have typing in @types, which is really convenient for the users - both the lib and typings are kept in sync via npm, and there's no need to ping-pong between npm and library repo. In fact, this is the second time I actually see something not publishing its typings to @types.

AGoblinKing · 2017-06-05T21:48:06Z

Why does it attempt to import flatbuffers from './flatbuffers' which assumes a relative location, rather than just 'flatbuffers' which would allow us to module resolution?

Ex: npm install --save flatbuffers => import { flatbuffers } from 'flatbuffers';

aardappel · 2017-06-05T22:31:43Z

@JoshGalvin I'm no JS/TS expert, but this may well have been inherited from the JS implementation. There were at least 2 attempts before (one is here: #3750) to address this, but the last had to be rolled back due to incompatibility with some environments I seem to remember (Google Closure compiler)?

krojew · 2017-06-06T06:01:37Z

This import is a leftover from a time where there was literally nothing TS-related for flatbuffers (from the first PR). You can disable it with a --no-fb-import flag.

AGoblinKing · 2017-06-06T17:53:58Z

I want it but just with 'flatbuffers' since I'm using node and webpack for module resolution. I made a fork with the change but yeah I saw that it gets reverted when people attempt that pull request. I can dig into the reasons and get it working w/ the errant environments. :sadness:

The namespacing is super weird when using them as modules too since everything would be exported on whatever namespace you provide (and broken if you don't provide a namespace).

import { Namespace } from './transform-generated';

Namespace.Transform.etc..

vs

import { Transform } from './transform-generated';

Edit:

An alternative approach to generating TS would be generating the .d.ts files for the javascript version. This would give type information to javascript users in modern IDEs and allow typescript users to use them directly.

fasterthanlime · 2017-06-06T18:01:36Z

The problem with having it just be 'flatbuffers' is that it forces users to be mindful of which version their package.json points to.

If they generate flatbuffers glue with a flatc version that's more recent than whatever their package.json resolves to, they'll have errors. Same for older flatc version. As long as there isn't a clear version compatibility contract between flatbuffers.js and the generated code, I think the TypeScript generate should generate both flatbuffers.js and flatbuffers.d.ts so that it's always in sync with the flatc version that generated the bindings.

AGoblinKing · 2017-06-06T18:13:15Z

That'd definitely meets my needs. The only thing better would be bundling flatc with the npm module like http://casperjs.org/. It attempts to build the binary and falls back on grabbing it if that fails.

Edit:
What happens when you want to generate hierarchy of flatbuffers? Would there be a flatbuffers.js in each level then?

krojew added 4 commits March 16, 2017 14:07

Eclipse ignore

919d467

TypeScript support

9f07d31

Prefixing enums

608653b

Test results

246048e

krojew added 5 commits March 21, 2017 09:00

Merge remote-tracking branch 'upstream/master'

291e3e5

Merged JS and TS generators

4684fe6

Fixed AppVeyor build problems

2627ba0

Fixed more AppVeyor build problems

8be41c4

Fixed more AppVeyor build problems

14b0986

fasterthanlime mentioned this pull request Mar 24, 2017

Typescript support (alternative) #4244

Closed

fasterthanlime suggested changes Mar 24, 2017

View reviewed changes

krojew added 3 commits March 27, 2017 08:17

Changed TS flag to options struct

00825db

Storing options by value

65d1a31

Removed unneeded const

8755370

aardappel reviewed Mar 27, 2017

View reviewed changes

fasterthanlime mentioned this pull request Apr 6, 2017

JavaScript: provide a standard way of creating longs from a number #4260

Closed

krojew added 2 commits April 7, 2017 20:01

Reverted generates js test file to original version

d9bfd6f

Backing up js tests and properly generating test data

a9c306c

aardappel merged commit 28e7dbd into google:master Apr 10, 2017

aardappel mentioned this pull request Jun 5, 2017

Typescript Support Early PR #4065

Closed

aardappel mentioned this pull request Aug 28, 2017

关于flatbuffers typescript #4418

Closed

asfimport mentioned this pull request Oct 18, 2017

[JS] Use flatc to generate TypeScript apache/arrow#16548

Closed

TypeScript support #4232

TypeScript support #4232

Uh oh!

Conversation

krojew commented Mar 17, 2017

Uh oh!

googlebot commented Mar 17, 2017

Uh oh!

krojew commented Mar 17, 2017

Uh oh!

aardappel commented Mar 18, 2017

Uh oh!

krojew commented Mar 18, 2017

Uh oh!

googlebot commented Mar 18, 2017

Uh oh!

aardappel commented Mar 18, 2017

Uh oh!

krojew commented Mar 18, 2017

Uh oh!

aardappel commented Mar 20, 2017

Uh oh!

fasterthanlime commented Mar 24, 2017

Uh oh!

fasterthanlime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fasterthanlime Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krojew commented Mar 24, 2017

Uh oh!

fasterthanlime commented Mar 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aardappel commented Mar 27, 2017

Uh oh!

fasterthanlime commented Mar 28, 2017

Uh oh!

krojew commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krojew commented Mar 28, 2017

Uh oh!

krojew commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

fasterthanlime Mar 25, 2017 •

edited

Loading

krojew commented Mar 28, 2017 •

edited

Loading

krojew commented Apr 6, 2017 •

edited

Loading

AGoblinKing commented Jun 6, 2017 •

edited

Loading

fasterthanlime commented Jun 6, 2017 •

edited

Loading

AGoblinKing commented Jun 6, 2017 •

edited

Loading