Typescript Support Early PR #4065

sampaioletti · 2016-10-20T09:37:36Z

Wouter had to give this a go it was driving me crazy thinking about it. I think for a rough first stab it shows it can work pretty well. I was able to code the FB tutorial with full type support using vscode (it was pretty awesome) and compile it to vanilla js. Didn't have a chance to generate tests or anything, just wanted to see how it would fit in so you could say how you wanted it completed. I'll play with it a little more and actually run a fb encode to see if it works (it should, or at least be close).

A couple notes

Couldnt make the IDLOptions workout cleanly so modified the constructor for the JSGenerator...may not be what you want, but it worked to get it going, I was just having to modify too much code to make the opts work right.
I tried to make a typings file for flatbuffers.js and it wasn't working for me (haven't written one before) so I just converted flatbuffers.js to a .ts file with all the typings, we can generate a d.ts from this so we can go back to the way you wanted to do it
I'm not sure if there is a more elegant way to do the generator code, its a lot of if/else nonesense but it got the job done
Compiled flatc with VS 2015 with no errors
The resulting project compiles nicely with tsc so I think its close to working for demo purposes

Not definitive by any means, I'm neither an expert at FB or TS, but was meant as a way to kick this off. The only time I get mad at FB is when I have to use it without intellisense....but that could be just me (: Let me know how/if you want me to keep going.

-Sam

googlebot · 2016-10-20T09:37:37Z

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.

sampaioletti · 2016-10-20T09:39:46Z

I signed it!

googlebot · 2016-10-20T09:39:47Z

CLAs look good, thanks!

fixed issues with 'this' not having intellisense, causing issue with generic in test_generated.ts line 220, tsc is dropping the namespace 'flatbuffers' off of 'flatbuffers.Table' and recognizing it as 2 different types

Caused issues all all around including with webpack/browserify

sampaioletti · 2016-10-20T22:30:33Z

Tested the flatbuffers.ts and generated.ts files in our project...worked like a champ

had initialized local variable with same name c\p bug

Make sure node doesn't grap the ts files

sampaioletti · 2016-10-21T16:55:20Z

Changed the example to be compatible with the one in the flatbuffers/tests area. Both the flatc->generated js file and the flatc->generatedts->tsc->generatedjs files pass the flatbuffers/tests/JavaScriptTest.js suite good first start.

Had to make the __union method generic so that calling method would know return value

fixed cast when type is boolean

Needs implementation check, just repeated writeIntXX methods

ghost

generally looks like the right direction, just needs a lot of cleanup :)

ghost · 2016-10-21T22:54:31Z

include/flatbuffers/code_generators.h

    return WrapInNameSpace(def.defined_namespace, def.name);
  }

+  std::string GetNameSpace(const Definition &def) {


please use spacing conform https://google.github.io/styleguide/cppguide.html

ghost · 2016-10-21T22:57:57Z

include/flatbuffers/code_generators.h


+  std::string GetNameSpace(const Definition &def) {
+  const Namespace *ns = def.defined_namespace;
+  const std::string &name = def.name;


Why was this needed? There's already the function FullNamespace

ghost · 2016-10-21T22:59:35Z

js/tsTest/JavaScriptTest.js

@@ -0,0 +1,350 @@
+// Run this using JavaScriptTest.sh


don't see any types in this file, why is there a copy?
tsTest -> ts_test

ghost · 2016-10-21T23:00:44Z

js/tsTest/monster_test.fbs

@@ -0,0 +1,80 @@
+// test schema file


please just use all the existing files rather than making copies.

ghost · 2016-10-21T23:04:17Z

src/idl_gen_js.cpp


-    return SaveFile(GeneratedFileName(path_, file_name_).c_str(), code, false);
+    std::string filename = "";
+    if (ts) {


yes, all these if-thens can use some cleanup. prefer to stick them in a table (see LanguageParameters in idl_gen_general.cpp for an example) where possible, and otherwise think how to represent the code with as few if-thens as possible by sticking them into functions where needed.

ghost · 2016-10-21T23:05:59Z

src/idl_gen_js.cpp

      code += "  }\n\n";
-      code += "  this.bb.write" + MakeCamel(GenType(field.value.type)) + "(this.bb_pos + offset, value);\n";
+      std::string value = "value";
+      if (GenTypeName(field.value.type, true)=="boolean") {


if (field.value.type.base_type == BASE_TYPE_BOOL)

sampaioletti · 2016-10-25T18:25:29Z

Thanks yeah it was a late night hackathon style was meant to just see how
it would go and then it worked haha. I'll go through and have a look at
your comments. I'll delete the duplicate files I just copied so I'd have
Temps to work with until ready.

On Oct 21, 2016 5:07 PM, "Wouter van Oortmerssen" notifications@github.com
wrote:

@gwvo commented on this pull request.

generally looks like the right direction, just needs a lot of cleanup :)

In include/flatbuffers/code_generators.h
#4065 (review)
:

@@ -112,6 +112,22 @@ class BaseGenerator {
return WrapInNameSpace(def.defined_namespace, def.name);
}

std::string GetNameSpace(const Definition &def) {

please use spacing conform https://google.github.io/

styleguide/cppguide.html

In include/flatbuffers/code_generators.h
#4065 (review)
:

@@ -112,6 +112,22 @@ class BaseGenerator {
return WrapInNameSpace(def.defined_namespace, def.name);
}

std::string GetNameSpace(const Definition &def) {

const Namespace *ns = def.defined_namespace;

const std::string &name = def.name;

Why was this needed? There's already the function FullNamespace

In js/tsTest/JavaScriptTest.js
#4065 (review)
:

@@ -0,0 +1,350 @@
+// Run this using JavaScriptTest.sh

don't see any types in this file, why is there a copy?

tsTest -> ts_test

In js/tsTest/monster_test.fbs
#4065 (review)
:

@@ -0,0 +1,80 @@
+// test schema file

please just use all the existing files rather than making copies.

In src/idl_gen_js.cpp
#4065 (review)
:

@@ -59,7 +65,14 @@ class JsGenerator : public BaseGenerator {
code += exports_code;
}

return SaveFile(GeneratedFileName(path_, file_name_).c_str(), code, false);

std::string filename = "";

if (ts) {

yes, all these if-thens can use some cleanup. prefer to stick them in a
table (see LanguageParameters in idl_gen_general.cpp for an example)
where possible, and otherwise think how to represent the code with as few

if-thens as possible by sticking them into functions where needed.

In src/idl_gen_js.cpp
#4065 (review)
:
   code += "  var offset = this.bb.__offset(this.bb_pos, " + NumToString(field.value.offset) + ")\n\n";
   code += "  if (offset === 0) {\n";
   code += "    return false;\n";
   code += "  }\n\n";
 code += "  this.bb.write" + MakeCamel(GenType(field.value.type)) + "(this.bb_pos + offset, value);\n";
 std::string value = "value";
 if (GenTypeName(field.value.type, true)=="boolean") {
if (field.value.type.base_type == BASE_TYPE_BOOL)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4065 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIZod67DYiQahjqA5XocvcebdQpygPUhks5q2UWTgaJpZM4Kb6mW
.

aardappel · 2017-01-05T00:00:13Z

Status?

sampaioletti · 2017-01-05T00:28:12Z

Used the ugly version in two projects that have been running in controlled production with no issues. Haven't had time to go back and do a final rewrite. On my list (: but it works very well.

…

On Jan 4, 2017 5:00 PM, "Wouter van Oortmerssen" ***@***.***> wrote: Status? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4065 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIZod3KdVfN6T6B7mmPNaxA2A72rZh1jks5rPDKWgaJpZM4Kb6mW> .

gwest7 · 2017-03-03T07:02:47Z

@sampaioletti is this still on your list?

sampaioletti · 2017-03-03T15:36:49Z

Yes unless someone else has time to twke it over ive just been slammed

…

On Mar 3, 2017 12:02 AM, "gwest7" ***@***.***> wrote: @sampaioletti <https://github.com/sampaioletti> is this still on your list? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4065 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIZod1BlKJD-AVuc252nWstlnP8UGyW9ks5rh7sggaJpZM4Kb6mW> .

aardappel · 2017-06-05T22:25:38Z

This is probably obsolete now that this was merged: #4232

sampaioletti added 3 commits October 20, 2016 03:21

TS Proof Of Concept flatc changes

3205124

TS Proof of Concept TS files

b768348

TS Proof of Concept tsc generated files

4e119a1

sampaioletti added 6 commits October 20, 2016 11:12

Corrected way TS defines member functions

28a6183

fixed issues with 'this' not having intellisense, causing issue with generic in test_generated.ts line 220, tsc is dropping the namespace 'flatbuffers' off of 'flatbuffers.Table' and recognizing it as 2 different types

Fixed mismarked static member

f30c9be

Made string accessor overloads to ease return type matching

a5f8e7d

Remove Default Exports

14290d9

Caused issues all all around including with webpack/browserify

Null check for Vector creation

dffeb4e

__string method overloads to detect return type

9cff070

sampaioletti added 7 commits October 21, 2016 08:49

Fixed bug in Vector of Struct generation

3587867

fixed bug in js generation

3416ad4

had initialized local variable with same name c\p bug

Fixed Formatting

b218fcf

Added Test files into folder for isolated testing

46bc307

bugfix TS vector generator

8adc8b5

bugfix mutate should not be static

570965d

Change file extensions

d03b8f6

Make sure node doesn't grap the ts files

sampaioletti added 3 commits October 21, 2016 11:22

Fixed Union typing issues

704d138

Had to make the __union method generic so that calling method would know return value

bugfix TS/JS boolean cast

68d1624

fixed cast when type is boolean

added scaffolding for writeUintXX methods

8eba6f4

Needs implementation check, just repeated writeIntXX methods

ghost reviewed Oct 21, 2016

View reviewed changes

first run

9cf3fd0

ghost force-pushed the master branch 3 times, most recently from c65126c to 2616516 Compare December 12, 2016 23:54

ghost force-pushed the master branch 11 times, most recently from 48a7fb3 to 2a7a44b Compare December 13, 2016 02:02

TheNeuralBit mentioned this pull request Mar 4, 2017

ARROW-613: WIP TypeScript Implementation apache/arrow#358

Closed

krojew mentioned this pull request Mar 18, 2017

TypeScript support #4232

Merged

fasterthanlime mentioned this pull request Mar 24, 2017

Typescript support (alternative) #4244

Closed

aardappel closed this Jun 5, 2017

Typescript Support Early PR #4065

Typescript Support Early PR #4065

Uh oh!

Conversation

sampaioletti commented Oct 20, 2016

Uh oh!

googlebot commented Oct 20, 2016

Uh oh!

sampaioletti commented Oct 20, 2016

Uh oh!

googlebot commented Oct 20, 2016

Uh oh!

sampaioletti commented Oct 20, 2016

Uh oh!

sampaioletti commented Oct 21, 2016

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost Oct 21, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Oct 21, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Oct 21, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Oct 21, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Oct 21, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Oct 21, 2016

Choose a reason for hiding this comment

Uh oh!

sampaioletti commented Oct 25, 2016

generally looks like the right direction, just needs a lot of cleanup :)

styleguide/cppguide.html

Why was this needed? There's already the function FullNamespace

tsTest -> ts_test

please just use all the existing files rather than making copies.

if-thens as possible by sticking them into functions where needed.

Uh oh!

aardappel commented Jan 5, 2017

Uh oh!

sampaioletti commented Jan 5, 2017 via email

Uh oh!

gwest7 commented Mar 3, 2017

Uh oh!

sampaioletti commented Mar 3, 2017 via email

Uh oh!

aardappel commented Jun 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants