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

[feature request] Proper packaging for JS ESM builds #1144

Closed
mshick opened this issue Dec 3, 2024 · 7 comments · Fixed by #1163
Closed

[feature request] Proper packaging for JS ESM builds #1144

mshick opened this issue Dec 3, 2024 · 7 comments · Fixed by #1163
Assignees
Labels
enhancement New feature or request language: js Related to JavaScript or Typescript integration

Comments

@mshick
Copy link

mshick commented Dec 3, 2024

Is your feature request related to a problem? Please describe.
When looking at the dist code on npm for @arizeai/openinference-vercel and the various other @arizeai/* packages, I noticed the esm builds appear incorrect. The builds are using extension-less ESM-style imports and exports, e.g., export { isOpenInferenceSpan } from "./utils";, and they do not declare "type": "module". in the package.json file. These modules won't work in a true ESM environment, and importing the package from a .mjs file, relies on the cjs build at dist/src.

Most users are probably importing into a TS environment with a bundler, and the only thing they'd miss out on here is tree shaking, but the fix is relatively simple so you might be interested in updating your boilerplate to do the following:

  1. Use extensions in all your local file imports import { foo } from './util' -> import { foo } from './util.js'
  2. Ensure the dist/esm folder has a package.json with { "type": "module" }. An npm script like this would do the trick:
"postbuild": "echo '{\"type\": \"module\"}' > ./dist/esm/package.json"
  1. Set an exports statement in your root package.json, e.g.,:
  "exports": {
    ".": {
      "import": "./dist/esm/index.js",
      "require": "./dist/src/index.js",
      "default": "./dist/src/index.js"
    }
  },

Another nit, @opentelemetry/sdk-trace-base seems to be required for @arizeai/openinference-vercel, but it is only listed as a devDependency.

I'd be happy to open a PR if you're interested.

@mshick mshick added enhancement New feature or request triage Issues that require triage labels Dec 3, 2024
@github-project-automation github-project-automation bot moved this to 📘 Todo in phoenix Dec 3, 2024
@dosubot dosubot bot added the language: js Related to JavaScript or Typescript integration label Dec 3, 2024
@mikeldking
Copy link
Contributor

@mshick thanks for catching this! We'll get it sorted. Thanks again!

https://publint.dev/@arizeai/[email protected]

@mikeldking mikeldking removed the triage Issues that require triage label Dec 4, 2024
@cephalization cephalization moved this from 📘 Todo to 👨‍💻 In progress in phoenix Dec 4, 2024
@cephalization
Copy link
Contributor

After investigation we will additionally need to upgrade typescript to 5.7 and use this flag https://devblogs.microsoft.com/typescript/announcing-typescript-5-7/#path-rewriting-for-relative-paths

Otherwise nodejs will not be able to resolve our emitted imports in esm mode, as they do not include file extensions.

@mikeldking
Copy link
Contributor

https://github.com/justkey007/tsc-alias as a possible solution

@mshick
Copy link
Author

mshick commented Dec 10, 2024

Packaging ESM using only TSC (no bundler) has long been a real pain in the butt. I tried a few tool-based solutions, but found that just adding .js extensions to .ts file imports was the easiest (even though I find it non-sensical). That's the approach I've seen used on popular ESM-only packages, like got — example.

@cephalization
Copy link
Contributor

I've got a PoC implemented following your suggestions w.r.t package.json, while fixing the file extension issues with tsc-alias, keeping the build process pretty slim without bundling.

I agree the file extension issue is interesting but makes some sense when you dive into the ESM specification 😅

Will bump when we've got something merged, thanks again for the shout!

@github-project-automation github-project-automation bot moved this from 👨‍💻 In progress to ✅ Done in phoenix Dec 12, 2024
@cephalization
Copy link
Contributor

@mshick ESM builds are now properly packaged and shipped for all openinference packages. Let us know if you see any issues using ESM.

So far it is known that auto-instrumentation does not work due to differences in opentelemetry when using cjs vs esm but we will look into this in the future. Manual instrumentation works as normal

@mshick
Copy link
Author

mshick commented Dec 13, 2024

Nice, thanks for heads up. Just took a peek at the updated packages and they look good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request language: js Related to JavaScript or Typescript integration
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants