Skip to content

Commit

Permalink
THRIFT-5811: Add ESM support to nodejs codegen
Browse files Browse the repository at this point in the history 
Client: nodejs

This adds a flag to the JS generator to output ES modules instead of CommonJS. This is only valid when targeting node. A lot of the changes here are to test this.

The `testAll.sh` script now generates an ES module version of the services and types, and tests the client and the server with these. This has a few knock-on effects. Firstly, any module that imports a generated ES module must itself be an ES module, since CommonJS modules cannot import ES modules. ES modules also do not support `NODE_PATH`, so instead the tests directory is converted into a node package with a `file:` dependency on the root thrift package.
  • Loading branch information
@cameron-martin
cameron-martin committed Jan 7, 2025
1 parent 0825ca3 commit 276325d
Show file tree
Hide file tree
Showing 11 changed files with 260 additions and 119 deletions.
145 changes: 102 additions & 43 deletions compiler/cpp/src/thrift/generate/t_js_generator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ class t_js_generator : public t_oop_generator {
gen_jquery_ = false;
gen_ts_ = false;
gen_es6_ = false;
gen_esm_ = false;
gen_episode_file_ = false;

bool with_ns_ = false;
Expand All @@ -83,6 +84,8 @@ class t_js_generator : public t_oop_generator {
with_ns_ = true;
} else if( iter->first.compare("es6") == 0) {
gen_es6_ = true;
} else if ( iter->first.compare("esm") == 0) {
gen_esm_ = true;
} else if( iter->first.compare("imports") == 0) {
parse_imports(program, iter->second);
} else if (iter->first.compare("thrift_package_output_directory") == 0) {
Expand All @@ -105,6 +108,10 @@ class t_js_generator : public t_oop_generator {
throw std::invalid_argument("invalid switch: [-gen js:with_ns] is only valid when using node.js");
}

if (!gen_node_ && gen_esm_) {
throw std::invalid_argument("invalid switch: [-gen js:esm] is only valid when using node.js");
}

// Depending on the processing flags, we will update these to be ES6 compatible
js_const_type_ = "var ";
js_let_type_ = "var ";
Expand Down Expand Up @@ -278,13 +285,6 @@ class t_js_generator : public t_oop_generator {
return js_namespace(p);
}

std::string js_export_namespace(t_program* p) {
if (gen_node_) {
return "exports.";
}
return js_namespace(p);
}

bool has_js_namespace(t_program* p) {
if (no_ns_) {
return false;
Expand Down Expand Up @@ -374,6 +374,11 @@ class t_js_generator : public t_oop_generator {
*/
bool gen_es6_;

/**
* True if we should generate ES modules, instead of CommonJS.
*/
bool gen_esm_;

/**
* True if we will generate an episode file.
*/
Expand Down Expand Up @@ -452,7 +457,7 @@ void t_js_generator::init_generator() {
f_episode_.open(f_episode_file_path);
}

const auto f_types_name = outdir + program_->get_name() + "_types.js";
const auto f_types_name = outdir + program_->get_name() + "_types" + (gen_esm_ ? ".mjs" : ".js");
f_types_.open(f_types_name.c_str());
if (gen_episode_file_) {
const auto types_module = program_->get_name() + "_types";
Expand All @@ -478,7 +483,13 @@ void t_js_generator::init_generator() {
}

if (gen_node_) {
f_types_ << js_const_type_ << "ttypes = module.exports = {};" << '\n';
if (gen_esm_) {
// Import the current module, so we can reference it as ttypes. This is
// fine in ESM, because it allows circular imports.
f_types_ << "import * as ttypes from './" + program_->get_name() + "_types.mjs';" << '\n';
} else {
f_types_ << js_const_type_ << "ttypes = module.exports = {};" << '\n';
}
}

string pns;
Expand Down Expand Up @@ -507,12 +518,26 @@ void t_js_generator::init_generator() {
*/
string t_js_generator::js_includes() {
if (gen_node_) {
string result = js_const_type_ + "thrift = require('thrift');\n"
+ js_const_type_ + "Thrift = thrift.Thrift;\n";
string result;

if (gen_esm_) {
result += "import { Thrift } from 'thrift';\n";
} else {
result += js_const_type_ + "thrift = require('thrift');\n"
+ js_const_type_ + "Thrift = thrift.Thrift;\n";
}
if (!gen_es6_) {
result += js_const_type_ + "Q = thrift.Q;\n";
if (gen_esm_) {
result += "import { Q } from 'thrift';\n";
} else {
result += js_const_type_ + "Q = thrift.Q;\n";
}
}
if (gen_esm_) {
result += "import Int64 from 'node-int64';";
} else {
result += js_const_type_ + "Int64 = require('node-int64');\n";
}
result += js_const_type_ + "Int64 = require('node-int64');\n";
return result;
}
string result = "if (typeof Int64 === 'undefined' && typeof require === 'function') {\n " + js_const_type_ + "Int64 = require('node-int64');\n}\n";
Expand Down Expand Up @@ -594,7 +619,7 @@ string t_js_generator::get_import_path(t_program* program) {
return "./" + import_file_name;
}

const string import_file_name_with_extension = import_file_name + ".js";
const string import_file_name_with_extension = import_file_name + (gen_esm_ ? ".mjs" : ".js");

auto module_name_and_import_path_iterator = module_name_2_import_path.find(import_file_name);
if (module_name_and_import_path_iterator != module_name_2_import_path.end()) {
Expand Down Expand Up @@ -638,7 +663,11 @@ void t_js_generator::generate_typedef(t_typedef* ttypedef) {
* @param tenum The enumeration
*/
void t_js_generator::generate_enum(t_enum* tenum) {
f_types_ << js_type_namespace(tenum->get_program()) << tenum->get_name() << " = {" << '\n';
if (gen_esm_) {
f_types_ << "export const " << tenum->get_name() << " = {" << '\n';
} else {
f_types_ << js_type_namespace(tenum->get_program()) << tenum->get_name() << " = {" << '\n';
}

if (gen_ts_) {
f_types_ts_ << ts_print_doc(tenum) << ts_indent() << ts_declare() << "enum "
Expand Down Expand Up @@ -680,7 +709,11 @@ void t_js_generator::generate_const(t_const* tconst) {
string name = tconst->get_name();
t_const_value* value = tconst->get_value();

f_types_ << js_type_namespace(program_) << name << " = ";
if (gen_esm_) {
f_types_ << "export const " << name << " = ";
} else {
f_types_ << js_type_namespace(program_) << name << " = ";
}
f_types_ << render_const_value(type, value) << ";" << '\n';

if (gen_ts_) {
Expand Down Expand Up @@ -859,9 +892,18 @@ void t_js_generator::generate_js_struct_definition(ostream& out,
vector<t_field*>::const_iterator m_iter;

if (gen_node_) {
string commonjs_export = "";

if (is_exported) {
if (gen_esm_) {
out << "export ";
} else {
commonjs_export = " = module.exports." + tstruct->get_name();
}
}

string prefix = has_js_namespace(tstruct->get_program()) ? js_namespace(tstruct->get_program()) : js_const_type_;
out << prefix << tstruct->get_name() <<
(is_exported ? " = module.exports." + tstruct->get_name() : "");
out << prefix << tstruct->get_name() << commonjs_export;
if (gen_ts_) {
f_types_ts_ << ts_print_doc(tstruct) << ts_indent() << ts_declare() << "class "
<< tstruct->get_name() << (is_exception ? " extends Thrift.TException" : "")
Expand Down Expand Up @@ -1198,7 +1240,7 @@ void t_js_generator::generate_js_struct_writer(ostream& out, t_struct* tstruct)
* @param tservice The service definition
*/
void t_js_generator::generate_service(t_service* tservice) {
string f_service_name = get_out_dir() + service_name_ + ".js";
string f_service_name = get_out_dir() + service_name_ + (gen_esm_ ? ".mjs" : ".js");
f_service_.open(f_service_name.c_str());
if (gen_episode_file_) {
f_episode_ << service_name_ << ":" << thrift_package_output_directory_ << "/" << service_name_ << '\n';
Expand Down Expand Up @@ -1282,7 +1324,11 @@ void t_js_generator::generate_service(t_service* tservice) {
<< tservice->get_extends()->get_name() << "');" << '\n';
}

f_service_ << js_const_type_ << "ttypes = require('./" + program_->get_name() + "_types');" << '\n';
if (gen_esm_) {
f_service_ << "import * as ttypes from './" + program_->get_name() + "_types.mjs';" << '\n';
} else {
f_service_ << js_const_type_ << "ttypes = require('./" + program_->get_name() + "_types');" << '\n';
}
}

generate_service_helpers(tservice);
Expand Down Expand Up @@ -1317,27 +1363,28 @@ void t_js_generator::generate_service_processor(t_service* tservice) {
vector<t_function*> functions = tservice->get_functions();
vector<t_function*>::iterator f_iter;

if (gen_node_) {
string prefix = has_js_namespace(tservice->get_program()) ? js_namespace(tservice->get_program()) : js_const_type_;
f_service_ << prefix << service_name_ << "Processor = " << "exports.Processor";
if (gen_ts_) {
f_service_ts_ << '\n' << "declare class Processor ";
if (tservice->get_extends() != nullptr) {
f_service_ts_ << "extends " << tservice->get_extends()->get_name() << ".Processor ";
}
f_service_ts_ << "{" << '\n';
indent_up();
std::string service_var;
if (!gen_node_ || has_js_namespace(tservice->get_program())) {
service_var = js_namespace(tservice->get_program()) + service_name_ + "Processor";
f_service_ << service_var;
} else {
service_var = service_name_ + "Processor";
f_service_ << js_const_type_ << service_var;
};
if (gen_node_ && gen_ts_) {
f_service_ts_ << '\n' << "declare class Processor ";
if (tservice->get_extends() != nullptr) {
f_service_ts_ << "extends " << tservice->get_extends()->get_name() << ".Processor ";
}
f_service_ts_ << "{" << '\n';
indent_up();

if(tservice->get_extends() == nullptr) {
f_service_ts_ << ts_indent() << "private _handler: object;" << '\n' << '\n';
}
f_service_ts_ << ts_indent() << "constructor(handler: object);" << '\n';
f_service_ts_ << ts_indent() << "process(input: thrift.TProtocol, output: thrift.TProtocol): void;" << '\n';
indent_down();
if(tservice->get_extends() == nullptr) {
f_service_ts_ << ts_indent() << "private _handler: object;" << '\n' << '\n';
}
} else {
f_service_ << js_namespace(tservice->get_program()) << service_name_ << "Processor = "
<< "exports.Processor";
f_service_ts_ << ts_indent() << "constructor(handler: object);" << '\n';
f_service_ts_ << ts_indent() << "process(input: thrift.TProtocol, output: thrift.TProtocol): void;" << '\n';
indent_down();
}

bool is_subclass_service = tservice->get_extends() != nullptr;
Expand Down Expand Up @@ -1419,6 +1466,12 @@ void t_js_generator::generate_service_processor(t_service* tservice) {
if (gen_node_ && gen_ts_) {
f_service_ts_ << "}" << '\n';
}

if(gen_esm_) {
f_service_ << "export { " << service_var << " as Processor };" << '\n';
} else {
f_service_ << "exports.Processor = " << service_var << ";" << '\n';
}
}

/**
Expand Down Expand Up @@ -1702,9 +1755,10 @@ void t_js_generator::generate_service_client(t_service* tservice) {

bool is_subclass_service = tservice->get_extends() != nullptr;

string client_var = js_namespace(tservice->get_program()) + service_name_ + "Client";
if (gen_node_) {
string prefix = has_js_namespace(tservice->get_program()) ? js_namespace(tservice->get_program()) : js_const_type_;
f_service_ << prefix << service_name_ << "Client = " << "exports.Client";
string prefix = has_js_namespace(tservice->get_program()) ? "" : js_const_type_;
f_service_ << prefix << client_var;
if (gen_ts_) {
f_service_ts_ << ts_print_doc(tservice) << ts_indent() << ts_declare() << "class "
<< "Client ";
Expand All @@ -1714,8 +1768,7 @@ void t_js_generator::generate_service_client(t_service* tservice) {
f_service_ts_ << "{" << '\n';
}
} else {
f_service_ << js_namespace(tservice->get_program()) << service_name_
<< "Client";
f_service_ << client_var;
if (gen_ts_) {
f_service_ts_ << ts_print_doc(tservice) << ts_indent() << ts_declare() << "class "
<< service_name_ << "Client ";
Expand Down Expand Up @@ -2204,6 +2257,12 @@ void t_js_generator::generate_service_client(t_service* tservice) {
indent_down();
f_service_ << "};" << '\n';
}

if(gen_esm_) {
f_service_ << "export { " << client_var << " as Client };" << '\n';
} else if(gen_node_) {
f_service_ << "exports.Client = " << client_var << ";" << '\n';
}
}

std::string t_js_generator::render_recv_throw(std::string var) {
Expand Down
2 changes: 1 addition & 1 deletion lib/nodejs/test/binary.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

const test = require("tape");
const binary = require("thrift/binary");
const binary = require("thrift/lib/nodejs/lib/thrift/binary");

const cases = {
"Should read signed byte": function(assert) {
Expand Down
19 changes: 9 additions & 10 deletions lib/nodejs/test/client.js → lib/nodejs/test/client.mjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,15 @@
* under the License.
*/

const assert = require("assert");
const thrift = require("thrift");
const helpers = require("./helpers");
import assert from "assert";
import thrift from "thrift";
import helpers from "./helpers.js";

const ThriftTest = require(`./${helpers.genPath}/ThriftTest`);
const ThriftTestDriver = require("./test_driver").ThriftTestDriver;
const ThriftTestDriverPromise = require("./test_driver")
.ThriftTestDriverPromise;
const SecondService = require(`./${helpers.genPath}/SecondService`);
const ThriftTest = await import(`./${helpers.genPath}/ThriftTest.${helpers.moduleExt}`);
import { ThriftTestDriver, ThriftTestDriverPromise } from "./test_driver.mjs";
const SecondService = await import(`./${helpers.genPath}/SecondService.${helpers.moduleExt}`);

const program = require("commander");
import program from "commander";

program
.option(
Expand All @@ -55,6 +53,7 @@ program
)
.option("--es6", "Use es6 code")
.option("--es5", "Use es5 code")
.option("--esm", "Use es modules")
.parse(process.argv);

const host = program.host;
Expand Down Expand Up @@ -172,4 +171,4 @@ function runTests() {
});
}

exports.expressoTest = function() {};
export const expressoTest = function() {};
39 changes: 34 additions & 5 deletions lib/nodejs/test/helpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

"use strict";
const thrift = require("../lib/thrift");
const thrift = require("thrift");

module.exports.transports = {
buffered: thrift.TBufferedTransport,
Expand All @@ -32,7 +32,36 @@ module.exports.protocols = {
header: thrift.THeaderProtocol
};

module.exports.ecmaMode = process.argv.includes("--es6") ? "es6" : "es5";
module.exports.genPath = process.argv.includes("--es6")
? "gen-nodejs-es6"
: "gen-nodejs";
const variant = (function() {
if (process.argv.includes("--es6")) {
return "es6";
} else if (process.argv.includes("--esm")) {
return "esm";
} else {
return "es5";
}
})();

module.exports.ecmaMode = ["esm", "es6"].includes(variant) ? "es6" : "es5";
const genPath = module.exports.genPath = (function() {
if (variant == "es5") {
return "gen-nodejs";
} else {
return `gen-nodejs-${variant}`;
}
})();

const moduleExt = module.exports.moduleExt = variant === "esm" ? "mjs" : "js";

/**
* Imports a types module, correctly handling the differences in esm and commonjs
*/
module.exports.importTypes = async function(filename) {
const typesModule = await import(`./${genPath}/${filename}.${moduleExt}`);

if (variant === "esm") {
return typesModule;
} else {
return typesModule.default;
}
}
Loading

0 comments on commit 276325d

Please sign in to comment.