THRIFT-5811: Add ESM support to nodejs codegen

[cameron-martin]

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.

• Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
• Did you squash your changes to a single commit? (not required, but preferred)
• Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
• If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

[thomasbruggink]

Thanks for this, we were actually planning to add something similar but for the browser. We can make a follow up PR to add support for esm on the browser side as well.

While testing this locally we noticed import statements are not changed to the esm format.
The existing PR keeps the require(type) style instead of changing it to the esm format of:

import * as <Identifier>_ttypes from <Identifier>.mjs 

(in t_js_generator.cc line 584)

Other than that it worked as expected 👍

cc: @tri613

[cameron-martin]

Thanks for this, we were actually planning to add something similar but for the browser. We can make a follow up PR to add support for esm on the browser side as well.

That's interesting, because this is our exact use case, but we use the nodejs package in the browser. Is your plan here to support es modules that can directly execute in the browser, with no build tools at all?

While testing this locally we noticed import statements are not changed to the esm format.

I must have missed this, I'll push an update. It also will need syncing with the nodejs CI changes (#3082).

[cameron-martin]

While testing this locally we noticed import statements are not changed to the esm format.

How do you exercise this code path? All of the tests files inside gen-nodejs-esm use import rather than require. Is this episodic generation that triggers this?

[tri613]

Hello! Thanks for this great PR again. 👍

import statements are not changed to the esm format

We tried to use this PR's changes to build our own service, and when looking inside the generated files, we noticed that the import statements are like this:

// FooService.mjs
import { Thrift } from 'thrift';
import { Q } from 'thrift';
import Int64 from 'node-int64';
var Foo_ttypes = require('./Foo_types');
var Bar_ttypes = require('./Bar_types');

As @thomasbruggink suggested we think this is related to the render_includes function at line 584.

Is your plan here to support es modules that can directly execute in the browser, with no build tools at all?

We had some issues when integrating the nodejs thrift build into our project, so we thought it would be nice if we could decouple the esm build from the nodejs dependencies. And of course, supporting native esm browser packages would be beneficial too :D
However, it is good to know that the nodejs version worked for you - I will take another look and check if we are missing anything in our project's build tool!

[cameron-martin]

We tried to use this PR's changes to build our own service, and when looking inside the generated files, we noticed that the import statements are like this:

Does this happen when a thrift file includes another thrift file? I don't think this is something the tests are covering atm. I'll investigate and add a test for it.

We had some issues when integrating the nodejs thrift build into our project, so we thought it would be nice if we could decouple the esm build from the nodejs dependencies. And of course, supporting native esm browser packages would be beneficial too :D
However, it is good to know that the nodejs version worked for you - I will take another look and check if we are missing anything in our project's build tool!

It worked for us by polyfilling a few NodeJS dependencies (such as util, Buffer, etc), but I've been wanting to remove the dependencies on these since in modern JS environments there are cross-runtime alternatives. See THRIFT-5844. In fact we have a few PRs in the works that should remove these.

Another part that is missing is browser tests for the NodeJS package, but I also have plans to contribute these too.

This does call into question the necessity of a separate nodejs and js package. Does anyone do web development without a bundler currently?

[cameron-martin]

@tri613 @thomasbruggink I've updated this to render includes correctly, plus added a test (include.test.mjs) for this.

However, CI will fail on this until #3087 is merged, since the current version of eslint and prettier do not support the dynamic import syntax, and the nodejs tests now rely on this.

[thomasbruggink]

This does call into question the necessity of a separate nodejs and js package. Does anyone do web development without a bundler currently?

At the moment we rely on the web version while also having a bundler in place. It looks to me like the web version is a leaner but less feature rich (although using some older standards like xmlhttprequest).
I haven't checked this into detail but I wonder if there is a benefit to keeping the more lightweight web version around 🤔

[thomasbruggink]

i have no power in the repository but we've tested this and it works for us ^^
thanks @cameron-martin for making these changes