privacy-scaling-explorations / zk-kit

A monorepo of reusable libraries for zero-knowledge technologies.
https://zkkit.pse.dev
MIT License
290 stars 76 forks source link

Non-ESM-compliant exports break for certain downstream consumers #319

Closed robknight closed 2 months ago

robknight commented 2 months ago

Describe the bug Modern ESM-compliant code requires that all import statements give a path to the file being imported including a file extension (typically .js). This is because ESM modules should be importable in a browser context, e.g. import "./path/to/import/index.js" maps to HTTP GET /path/to/import/index.js. Unlike filesystem-based imports, browsers cannot cheaply try multiple file extensions in order to locate the file, so full file extensions are mandatory in ESM imports.

However, older TypeScript code which uses moduleResolution: "bundler" often contains imports without filenames, e.g. import Component from "./Component" rather than import Component from "/.Component.js". This affects both JavaScript output and TypeScript type definitions (.d.ts files).

With the newer NodeNext module resolution strategy, TypeScript will not allow non-standard imports which do not have the file extension. Importing from a package with "incorrect" imports will break. This applies to both JavaScript and TypeScript definition imports.

If a downstream project is an ESM module ("type": "module" in package.json) and is using "moduleResolution": "NodeNext" in tsconfig.json, then imports from ZK-kit packages will not work properly, even if those packages could be imported successfully with "moduleResolution": "bundler". This is because the TypeScript definition files have non-ESM-compliant imports, e.g.:

export * from "./eddsa-poseidon";
export * from "./types";

To Reproduce There is a reproduction of the issue here: https://github.com/robknight/esm-import-repro Clone the repo and run pnpm build, and you should see this error message:

src/index.ts:1:10 - error TS2305: Module '"@zk-kit/eddsa-poseidon"' has no exported member 'derivePublicKey'.

1 import { derivePublicKey } from "@zk-kit/eddsa-poseidon";
           ~~~~~~~~~~~~~~~

Expected behavior It is clearly intended that this function (and others!) should be exported. However, TypeScript cannot follow the imports from dist/types/index.d.ts because they lack file extensions.

I could switch my project to use a bundler, and "moduleResolution": "bundler", but since I want to create a package which can be consumed by other people, I would simply be pushing the problem to them: they would also be forced to use "moduleResolution": "bundler" and would be unable to use "moduleResolution": "NodeNext" when importing my package.

One fix is to include file extensions in the TypeScript source, e.g. export * from "./eddsa-poseidon.js"; export * from "./types/index.js";. Note that the file extension should be .js even though the source file is .ts.

In this specific case, another option would be to bundle all of your TypeScript definitions into a single output file. The rollup dts plugin can do this. Because there's only one file, there are no import statements, and so they can't be non-compliant.

Screenshots If applicable, add screenshots to help explain your problem.

Technologies (please complete the following information):

Additional context https://nodejs.org/api/esm.html#esm_packages https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c https://www.reddit.com/r/typescript/comments/1b87o96/esm_on_nodejs_file_extension_mandatory/

cedoor commented 2 months ago

Hey @robknight, thanks for pointing this out!

IMO dts is the simplest and cleanest solution.

Wdyt?

robknight commented 2 months ago

Hey @robknight, thanks for pointing this out!

IMO dts is the simplest and cleanest solution.

Wdyt?

That sounds good to me!

cedoor commented 2 months ago

@robknight Could you review https://github.com/privacy-scaling-explorations/zk-kit/pull/320?