dd-trace-js icon indicating copy to clipboard operation
dd-trace-js copied to clipboard

Incorrect exports for TypeScript in ESM mode

Open benasher44 opened this issue 2 years ago • 12 comments

Expected behaviour import tracer from "dd-trace"; works in TypeScript ESM mode

Actual behaviour Instead of tracer referring to the tracer, an extra .default access is required.

Steps to reproduce See https://arethetypeswrong.github.io/?p=dd-trace%404.2.0 for a description of the issue and suggested fix. A TS project with moduleResolution set to "NodeNext" and package.json's "type" set to "module" reproduces it.

Environment

  • Operation system: macOS 13.4
  • Node.js version: node 18.x
  • Tracer version: 4.2.0
  • Agent version:
  • Relevant library versions:

benasher44 avatar Jun 20 '23 15:06 benasher44

@benasher44 would you be willing to make a pull request for this?

khanayan123 avatar Aug 29 '23 17:08 khanayan123

Closing due to inactivity, if you are still having this problem please feel free to reopen

khanayan123 avatar Sep 06 '23 17:09 khanayan123

This is still an issue. I would consider making a PR, but it doesn't look like I can reopen this issue.

benasher44 avatar Sep 10 '23 15:09 benasher44

@khanayan123 can you please reopen? This is still an issue. The TypeScript ESM support is still incorrect.

benasher44 avatar Jan 04 '24 17:01 benasher44

Hi @andrewbranch! I'm not sure what to recommend here. The tracer export has the requisite default property on it, so it should work to export = tracer, but I'm not sure how to represent the rest of named exports alongside the default export. I looked at the new module resolution guides published in 5.3, but I haven't been able to figure it out. Any tips would be appreciated!

benasher44 avatar Jan 04 '24 17:01 benasher44

arethetypeswrong isn't able to find that default property assignment, so the docs it links too aren't the most helpful. The actual problem is this one, which explains how to combine named exports of types with an export =: https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/MissingExportEquals.md

Let me know if that provides enough to get you started.

andrewbranch avatar Jan 04 '24 17:01 andrewbranch

Ah, thanks @andrewbranch!

@khanayan123 is that enough info for your team to fix? (started working on it myself, but it turned out to be more significant than I think it makes sense for an outside contributor)

benasher44 avatar Jan 04 '24 18:01 benasher44

@khanayan123 I took a crack at it in #3937, but there's one outstanding issue. @andrewbranch I have the namespace merging set up, but when I attempt to use it in code, it doesn't seem to resolve the tracer/default export as Tracer the interface (resolves to Tracer to the namespace — maybe this isn't right though and I'm misunderstanding the root issue). For example, I can't call tracer.init

benasher44 avatar Jan 05 '24 21:01 benasher44

It’s a little hard to tell in a big diff for a library I’m not familiar with, but it looks like you have something like this:

// namespace is also a value because it exports a value
declare namespace Tracer {
  // export some types
  export interface Foo {}
  // export a value
  export const tracer: Tracer;
  export { tracer as default };
}

// namespace/value merges with this type
interface Tracer {}

// export type, value, namespace merge
export = Tracer;

Notice that the export = is not a value tracer: Tracer. It is a value more like { tracer: Tracer, default: Tracer }. You need to make the value meaning of the top-level export = have the Tracer type. You will probably need to make the interface contain only types, so it can merge with a top-level const tracer: Tracer value.

// namespace is type-only (aka uninstantiated)
// made it lowercase since main export is `tracer`
declare namespace tracer {
  // export some types only
  export interface Foo {}
}

// this type no longer merges, it's uppercase
interface Tracer {
  default: Tracer; // "default export" moves to this property
  // ...
}

// this value merges with the uninstantiated namespace
declare const tracer: Tracer;

// export value namespace merge
export = tracer;

andrewbranch avatar Jan 08 '24 19:01 andrewbranch

Thanks for the tip! That suggestion definitely makes sense (i.e. value vs type). I pushed an update and rearranged the diff a bit. It should be easier to read now with the "Hide Whitespace" setting enabled.

After that update, it seems it doesn't understand I want to merge the tracer const with the tracer namespace. I'm getting this error (similar error on the namespace declaration):

image

benasher44 avatar Jan 08 '24 20:01 benasher44

It looks like the namespace must still contain a value.

andrewbranch avatar Jan 08 '24 21:01 andrewbranch

You were right — sneaky one!

benasher44 avatar Jan 08 '24 21:01 benasher44