feat!: move @google-cloud/translate to ESM and Node 14

[sofisl]

A few notes on this move:

1. A special post-compile.js file was required to modify compiled commonJS code after transpiling from Typescript, since some of the code won't compile if the module type is set in the package.json. This post-compile script is a hacky proof-of-concept, but captures some important steps, for example:
   • Replacing instances of import.meta with __dirname
   • Using proxyquire instead of esmock
   • Renaming .js files to .cjs (in order for them to be interpreted as common JS)

If the idea of this script looks OK, I can clean it up.

2. Translate seems to be an outlier in that it uses the protos.js file in its tests, which should not be a load-bearing file. Protos.js uses UMD modules. I created a protos-hack.js file that outlines what this file would need to look like to be used in ESM. Perhaps we can change what this file would look like as its generated in gax (@alexander-fenster to weigh in).

3. Currently, we're only running unit tests on ESM files, and we're still using CJS to run sample & system tests. If we wanted to run system-tests on ESM, we could do so by providing options here and here to install it in an ESM package.

4. Since a .cjs file cannot just import an ESM file, we do need to copy over the protos files into each subdirectory (and rename them to .cjs in that directory), since we use it in our client.ts file. Perhaps @alexander-fenster can confirm this. The concern is that this would double our current library size (instead of just 1/3 as we'd hoped). --> nevermind, this is OK for all "regular" gapics. For translate, I think we'd still need #2 since they're using it in tests. Or we could refactor the tests.

5. Subsequently, some gax stuff expects there to be a build/protos file, instead of build/cjs/protos and build/esm/protos. This isn't a big deal since we can just add a directory argument (for example, to minifyProtosJson), but it might be worth considering changing the default behavior.

6. Some templates need to be changed in synthtool from js --> cjs.

7. Node 12 test will fail because this moves to Node 14. Once I see tests passing, I'll skip this module from being tested on Node 12.

[ddelgrosso1]

Can this be removed as well?

[sofisl]

I think we need it to run tests for CJS (since it doesn't support esmock)

[ddelgrosso1]

If we decide that using a post compile step is the best path forward it would probably make sense to extract this to some kind of utility repo so that it is reusable.

[ddelgrosso1]

Was there an issue with 14.0.0?

[sofisl]

good call, just using it while I was testing.

[ruyadorno]

More of a review of the idea implemented here and less on this specific implementation but conceptually I believe unit tests will benefit less from running on the multiple module system targets. In comparison end-to-end test tend to benefit more since they're testing that all parts together are working as expected.

[sofisl]

I agree! Unfortunately our system tests won't test ESM, since they use a testing harness called pack-n-play that essentially creates a CJS package and installs our libraries as dependencies. However, in the PR body I describe what we can do to modify that harness to test in ESM as well.

[bcoe]

What I think would be best would be to extend pack-n-play to test the ESM build of the package, at this point we could run unit tests against just ESM or CJS.

[sofisl]

Yep! That's what I suggest in #3 in the PR body!

[bcoe]

First pass of review. The main thing that jumped out at me is that I think we should pull the path helpers into a module. For the prototype, this could be in the packages/google-cloud-translate module.

I think if you do this refactor, you might be able to eliminate some of the regex post processing you're doing, also it will allow you to delete a lot of code.

[bcoe]

This regex seems pretty fragile, I'm betting when we update to a new typescript version at some point it will stop working silently. I'll leave a comment further down in code review.

[bcoe]

I would pull this logic into a separate utility library, perhaps in the generator or gax so that we don't have to add this in every generated library.

I think if we're creative with how this helper is implemented, we should be able to avoid the regexMetaInput replacement.

What I did in yargs was to have a different helper file for ESM vs., CJS, during the build step you can then use the appropriate helper. Such that the generated code only has __dirname for CJS and import.meta.url for ESM.

[sofisl]

I agree that the regex is fragile; it's meant to be a working prototype, but I do think regex is ultimately the way to go for the __dirname --> import.meta.url problem. Say, for example, we created a helper file that exports a default function called dirnameOrImportMeta(), that lives in gax, for example, that looks like this, and is imported in each file that asks for the __dirname:

export default function dirnameOrImportMeta() {
    let dirToUse = '';
    try {
      dirToUse = __dirname;
    } catch (e) {
      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
      // @ts-ignore
      const filename = fileURLToPath(import.meta.url);
      dirToUse = path.dirname(filename);
    }
  
    return dirToUse;
  }

This option is fragile in that __dirname would actually be pointing to where this particular file lives, not the file that is calling this function. This could lead to wrong paths. Even if we passed the context of the currently executing directory, such as process.cwd(), this also wouldn't necessarily be called from the file that is executing, since we wouldn't know what context that particular file is being run in.

Another option is to just use this logic in each file that needs it:

    let dirToUse = '';
    try {
      dirToUse = __dirname;
    } catch (e) {
      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
      // @ts-ignore
      const filename = fileURLToPath(import.meta.url);
      dirToUse = path.dirname(filename);
    }
  
    return dirToUse;

However, CJS tests will complain during runtime:

TypeError [ERR_INVALID_URL]: Invalid URL
    at __node_internal_captureLargerStackTrace (node:internal/errors:484:5)
    at new NodeError (node:internal/errors:393:5)
    at URL.onParseError (node:internal/url:565:9)
    at new URL (node:internal/url:645:5)
    at fileURLToPath (node:internal/url:1483:12)

Therefore, we'd still need to do some regex replacing to remove the filename part in build/cjs (which is what we're doing in this prototype, hence the fragility).

Another option, we could do something like:

const dirname = CHANGE_THIS

And change it to be what each file needs in the post-compiling step. However, this option wouldn't allow the file to be runnable (say, for example, from Deno), since it would need to be compiled to be executable.

Lastly, there are libraries that do this transition for you - like [tsup](https://www.npmjs.com/package/tsup), according to this article. The downsides of this alternative are that 1) we'd probably need other custom post-compiling done, like esmock --> proxyquire, and 2) we'd have another dependency that we'd have to maintain and probably does not overlap perfectly with what we need it to do.

[bcoe]

Thanks for doing this Sofia 🥳 left a first round of comments.