Feature: Expose esm hooks factory to public API

[nonara]

Hope you're doing well!

PR Scope

• Create public facing createEsmHooks method

Background

The transformer library typescript-transform-paths now supports ts-node without a Program instance via typescript-transform-paths/register script. This addition is relatively recent.

We've just received an issue from a user trying to make it work via esm. (see: LeDDGroup/typescript-transform-paths#134)

Because a NodeJS loader hook is used, our recreated instance isn't sufficient, as the loader remains connected to the initial configuration.

Solution + Problem

We've added a loader to make it work, which calls registerAndCreateEsmHooks, with the proper options. (see: LeDDGroup/typescript-transform-paths#135)

But, because the loader is an es module, we can't import from ts-node/dist/esm, because the file isn't in the ts-node exports section of package.json.

[cspotcode]

Exposing this API surface makes sense, but we should do it in a way that's clean and feels like it belongs in our public API surface. The changes should still be minimal.

Our existing APIs are register, create, createRepl, etc. To match this naming, how about createEsmLoader(tsNodeService) or createEsmHooks(tsNodeService)? This aligns with the API enhancements that will happen in #1429.

We can export this function from ts-node/esm or from ts-node. If we export from ts-node, we should still preserve the lazy-loading of ESM functionality, so that CommonJS consumers do not pay the performance cost for loading stuff only needed for ESM. We already do some lazy-loading / conditional require()s internally; nothing fancy or complicated.

[nonara]

Thanks for the response!

Exposing this API surface makes sense, but we should do it in a way that's clean and feels like it belongs in our public API surface.

Makes sense. I wasn't sure if you'd want to support as official API, so I thought it best to open with the path of least resistance.

How about createEsmLoader(tsNodeService) or createEsmHooks(tsNodeService)? This aligns with the API enhancements that will happen in #1429.

Sounds good.

We can export this function from ts-node/esm or from ts-node. If we export from ts-node, we should still preserve the lazy-loading of ESM functionality, so that CommonJS consumers do not pay the performance cost for loading stuff only needed for ESM. We already do some lazy-loading / conditional require()s internally; nothing fancy or complicated.

I assume by ts-node/esm, you're referring to ./esm.mjs. Because we need to register an instance in that file (being the loader), I opted to export from ts-node. I used a lazy-load approach with type safety. Let me know if it lines up with what you were thinking.

[nonara]

One final note — although the code is covered by existing tests, because it's now public-facing, you may want to add some level of test to validate the exported function createEsmHooks.

Also, nice work with the esm support!

[cspotcode]

Because we need to register an instance in that file (being the loader), I opted to export from ts-node

Good catch, agreed on this approach.

because it's now public-facing, you may want to add some level of test to validate the exported function createEsmHooks.

Yes, this PR will need tests before it can be merged.

[nonara]

I've been thinking this over a bit in terms of approach. Here's where I'm at.

You have a battery of tests which covers the loader. But, we need to guarantee that the coverage translates to a custom loader. I generally use jest, which allows for using each to repeat a suite of tests on a second target, mocking, etc. In this case, it seems we're pretty limited.

I believe that the best route for this is to:

1. Create a custom loader in assets (essentially replicating ./esm.mjs)
2. Mock createEsmHooks and register for the custom and the assets loader
3. For both – ensure that createEsmHooks is called with the created instance and that the module exports the proper returned values

This would provide complete coverage without any redundancy in tests.

With that said, I'm not seeing any precedent for mocking in the tests. I don't see sinon. I do see that proxyquire is installed but is not used in that capacity.

So my questions are:

1. Would you prefer not mocking?
2. If mocking is okay, is there a particular means you'd prefer?

There are really a number of ways that it could be done.

Realistically, you could even just use the compiler API to parse the esm.mjs source and verify that it is exporting the proper variables from the correct createEsmHooks function. Also confirming that it's taking a single parameter which is a service instance. (Edit: You'd also need to confirm the index.ts export)

Finally, there is another route which may be more in line with precedent, but it's less ideal, in my opinion.

You could create a separate loader in assets, which supplies something like { "compilerOptions": { "resolveJsonModule": true } } to the instance, where an index file would console.log an import from a json file. You then verify the std_error and std_out from the exec result. This would verify that it took the instance and created the hooks, but for obvious reasons, it's not quite as solid (because we can't definitively guarantee the link between the existing loader tests and it)

At any rate, I'm new to the codebase, and not very familiar with ava, so maybe I'm missing a better route. Will wait to hear your thoughts.

[cspotcode]

We package ts-node and then `npm install` it into ./tests/node_modules` so I think you can write a pretty simple integration / e2e-style test for this.

…

On Mon, Aug 23, 2021 at 1:54 PM Ron S. ***@***.***> wrote: I've been thinking this over a bit in terms of approach. Here's where I'm at. You have a battery of tests which covers the loader. But, we need to guarantee that the coverage translates to a custom loader. I generally use jest, which allows for using each to repeat a suite of tests on a second target, mocking, etc. In this case, it seems we're pretty limited. I believe that the best route for this is to: 1. Create a custom loader in assets (essentially replicating ./esm.mjs) 2. Mock createEsmHooks and register for the custom and the assets loader 3. For both – ensure that createEsmHooks is called with the created instance and that the module exports the proper returned values This would provide complete coverage without any redundancy in tests. With that said, I'm not seeing any precedent for mocking in the tests. I don't see sinon. I do see that proxyquire is installed but is not used in that capacity. So my questions are: 1. Would you prefer not mocking? 2. If mocking is okay, is there a particular means you'd prefer? There are really a number of ways that it could be done. Realistically, you could even just use the compiler API to parse the esm.mjs source and verify that it is exporting the proper variables from the correct createEsmHooks function, while also passing a created instance in a single parameter. Finally, there is another route which may be more in line with precedent, but it's less ideal, in my opinion. You could create a separate loader in assets, which supplies something like { "compilerOptions": { "resolveJsonModule": true } } to the instance, where an index file would console.log an import from a json file. You then verify the error and regular outputs. This would verify that it took the instance and created the hooks, but for obvious reasons, it's not quite as solid. At any rate, I'm new to the codebase, and not very familiar with ava, so maybe I'm missing a better route. Will wait to hear your thoughts. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1439 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC35OB3JEO5YFEEJYNMOELT6KDNBANCNFSM5CSKLR6A> .

[nonara]

Should the if be above the suite? Also, is the version logic sound?

[nonara]

Requested changes implemented and test added.

I may also have come across a bug (unrelated to this PR).

Given:

{
  "compilerOptions": {
     "resolveJsonModule": true
  }
}

With esm enabled, I encounter this while trying to import { name } from "./package.json"

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".json" for C:\Work\Projects\ts-node\tests\esm-custom-loader\package.json

I'm not extremely well versed in esm, so perhaps I should be using require for that? If you think it's aberrant, I'll file an issue.

[cspotcode]

You're probably hitting this: https://nodejs.org/dist/latest-v16.x/docs/api/esm.html#esm_no_json_module_loading

[cspotcode]

Note to self: refactor experimentalEsmLoader into an @internal method on the Service: enableExperimentalEsmLoaderInterop()

This avoids consumers needing to pass an @internal flag at service creation time, since we can call the method automatically within createEsmHooks.

This is a nitpicky thing, so I'm happy to do the work myself and push the changes to this branch.

[nonara]

Sounds good. Maintainer edits are enabled!

[nonara]

Just checking in. I have a couple of esm-related features that I'm adding in today. If you think this PR will be merged somewhat soon, I may hold off on the release to roll all in. No rush, of course, but if you have any idea on a timeline, I'd appreciate the heads up!

[cspotcode]

I'm pretty bad at time estimation, and my paycheck-job needs some attention this weekend, but how about I plan to merge and release this tomorrow? That way if I don't respond tomorrow you'll know I ran out of time and it'll likely be delayed a while longer.

[nonara]

@cspotcode No worries. I'm very much the same on my estimations! Working on that, myself. 😄 That sounds good. Thanks!

[codecov]

Codecov Report

Merging #1439 (99863cc) into main (b52ca45) will increase coverage by 0.10%.
The diff coverage is 93.33%.

Impacted Files	Coverage Δ
src/bin.ts	90.69% <ø> (ø)
src/configuration.ts	93.82% <ø> (ø)
src/index.ts	77.55% <80.00%> (+0.22%)	⬆️
src/esm.ts	88.13% <100.00%> (+0.63%)	⬆️

[cspotcode]

@nonara Thanks again for this contribution and for your patience, this is published in v10.3.0. Please let me know if you hit any bugs.

https://github.com/TypeStrong/ts-node/releases/tag/v10.3.0

[nonara]

Thank you! Looking forward to implementing it.