Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

THRIFT-3143 #488

Closed
wants to merge 11 commits into from
Closed

THRIFT-3143 #488

wants to merge 11 commits into from

Conversation

wilfrem
Copy link
Contributor

@wilfrem wilfrem commented May 11, 2015

Current typescript support is only work for browser, and generated d.ts uses internal module.
So, it's hard to use for typescript in node.js.

To solve probrem, generate typescript code directory.

@LoungeFlyZ
Copy link

This would be great to get in. Is anyone able to take a look at it?

@endel
Copy link

endel commented Jun 10, 2016

Thanks for your work @wilfrem. I think this pull-request should be reviewed and maybe accepted, since JavaScript is evolving so fast - which makes the current JavaScript/Node generators unusable in modern build tools for the browser, for example. Having a TypeScript generator could solve this problem.

I did some changes in the import section, which seems to be broken in the current version. Here's the patch to import the actual references needed:

diff --git a/compiler/cpp/src/generate/t_ts_generator.cc b/compiler/cpp/src/generate/t_ts_generator.cc
index 2100f7f..44fbeb5 100644
--- a/compiler/cpp/src/generate/t_ts_generator.cc
+++ b/compiler/cpp/src/generate/t_ts_generator.cc
@@ -343,9 +341,36 @@ string t_ts_generator::render_includes() {
   if (gen_node_) {
     const vector<t_program*>& includes = program_->get_includes();
     for (size_t i = 0; i < includes.size(); ++i) {
-      result += "import " + includes[i]->get_name() + "_ttypes = require('./" + includes[i]->get_name()
-                + "_types')\n";
+
+      // Import all exposed enums/structs/types from defined module
+      std::vector<std::string> imports;
+
+      const std::vector<t_typedef*>& typedefs = includes[i]->get_typedefs();
+      for (size_t j = 0; j < typedefs.size(); ++j) {
+        imports.push_back(typedefs[j]->get_type()->get_name());
+      }
+
+      const std::vector<t_enum*>& enums = includes[i]->get_enums();
+      for (size_t j = 0; j < enums.size(); ++j) {
+        imports.push_back(enums[j]->get_name());
+      }
+
+      const std::vector<t_struct*>& structs = includes[i]->get_structs();
+      for (size_t j = 0; j < structs.size(); ++j) {
+        imports.push_back(structs[j]->get_name());
+      }
+
+      std::stringstream modules;
+      for(size_t j = 0; j < imports.size(); ++j)
+      {
+        if(j != 0)
+          modules << ", ";
+        modules << imports[j];
+      }
+
+      result += "import { " + modules.str() + " } from './" + includes[i]->get_name() + ".ts'\n";
     }
+
     if (includes.size() > 0) {
       result += "\n";
     }

@endel
Copy link

endel commented Jun 10, 2016

My approach is not very clean as it simply ignores node.js existance, it is meant to run only in the browser: endel@3235744

@HIRANO-Satoshi
Copy link
Contributor

Does anyone merge it?

I think I need this.

@jeking3
Copy link
Contributor

jeking3 commented Jun 17, 2016

I'd recommend that you rebase your changes on the latest origin/master and push to make sure they are compatible with the development tip and pass all tests.

@estk
Copy link

estk commented Jul 13, 2016

Bump.
Is anyone working on this?

@jeking3
Copy link
Contributor

jeking3 commented Apr 11, 2017

I'd recommend that you rebase your changes on the latest origin/master and push to make sure they are compatible with the development tip and pass all tests.

@jeking3
Copy link
Contributor

jeking3 commented Jan 24, 2018

This pull request is being closed due to lack of activity.
You can re-open it after rebasing on the current master if you would like it to be reconsidered.

@asfgit asfgit closed this in 8d96b3b Jan 24, 2018
jeking3 added a commit to jeking3/thrift that referenced this pull request Mar 10, 2018
This closes apache#93
This closes apache#326
This closes apache#345
This closes apache#352
This closes apache#353
This closes apache#383
This closes apache#395
This closes apache#413
This closes apache#488
This closes apache#555
This closes apache#624
This closes apache#731
This closes apache#747
This closes apache#756
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants