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

Make it possible to use dd-trace-js together with a Typescript ESM loader #2190

Closed
fardjad opened this issue Jul 17, 2022 · 5 comments
Closed

Comments

@fardjad
Copy link

fardjad commented Jul 17, 2022

Thanks to (experimental) support for loaders in Node.js, it's relatively easy to run Typescript without a pre-compilation step in ESM projects. That simplifies the build process and improves the developer experience.

However, it's still hard to get the ESM loader used by dd-trace-js (import-in-the-middle) to play nice with a Typescript ESM loader such as ts-node.

I reported an issue here about two weeks ago, but I did not receive any acknowledgment/feedback/input from Datadog whatsoever. As we are planning to migrate our Node.js repositories to ESM, and we would like to keep the possibility of running our TS projects without a build step, I thought it's best to raise this here again in a more generic manner.

@rochdev
Copy link
Member

rochdev commented Jul 18, 2022

I opened nodejs/import-in-the-middle#16 to try and merge your test project and the extension fix as a single PR, but I wasn't able to make it work. Do you know what could cause the error here? I'm not familiar at all with ts-node so I'm not sure what's going on.

@fardjad
Copy link
Author

fardjad commented Jul 18, 2022

I think I don't have access to see the logs of the workflows, but I rebased my fork on your branch and updated the PR. It should be working now. ts-node/esm is still not chain-friendly in Node v18.6, because it sets the shortCircuit property. Until that changes (which IMHO, should not matter much for this project), we can create our own custom chain loader. I also had to rename say-hi.ts to say-hi.mts because esm mode is enabled for ts-node.

@rochdev
Copy link
Member

rochdev commented Jul 19, 2022

Perfect, thanks for the quick fix. Do you know why it's a problem if ts-node/esm uses shortCircuit since it comes last anyway?

@fardjad
Copy link
Author

fardjad commented Jul 19, 2022

No problem!

According to the official Node.js docs, if we do --loader loader1 --loader loader2, it actually calls loader2 first and then loader2 MUST call loader1 unless it returns shortCircuit: true.

After refreshing my memory on how loaders work, I realized that my previous comment was not accurate and while it's related, it doesn't have much to do with shortCircuit. When we pass multiple --loader arguments to node, the default resolve/load function will always be the one provided by node. Keeping that in mind, when we do --loader iitm --loader ts-node/esm, the following happens when importing a Typescript file:

  1. ts-node/esm::resolve resolves the specifier and automatically resolves files with JS extensions to their corresponding TS extensions, and that's why we need TS extensions support in IITM.
  2. ts-node/esm::load gets called, and then it hands over control to Node, which in turn calls iitm::load
  3. We end up with an endless recursion. iitm::load eventually invokes ts-node/esm::load and ts-node/esm:load passes control back to the default load function provided by Node, which in turn calls the next loader (i.e. iitm) and it goes on and on (What if we flip the order of loaders? we'll run into a similar scenario. Also, because of the way IITM is implemented, even if we fix the endless recursion problem, it still won't work)

The custom loader fixes this issue by composing resolve and load into single functions that do not require chaining

@rochdev
Copy link
Member

rochdev commented Jul 19, 2022

Sounds like this could be handled better in iitm. In any case, now that the new plugin system landed in dd-trace, it's very likely we'll end up with a custom internal loader instead which shouldn't have this type of issue in the following months.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants