Skip to content

o1js is not quite compatible with NodeNext TSC module resolution #1447

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

Open
ukstv opened this issue Feb 19, 2024 · 18 comments
Open

o1js is not quite compatible with NodeNext TSC module resolution #1447

ukstv opened this issue Feb 19, 2024 · 18 comments
Labels
bug Something isn't working

Comments

@ukstv
Copy link

ukstv commented Feb 19, 2024

If you use NodeNext module resolution setting,
an instance of ZkProgram can not be exported. TSC complains it can not actually infer types.

See reproduction case: https://github.com/ukstv/o1js-nodenext-repro

Probably related issue: microsoft/TypeScript#47663

@mitschabaude
Copy link
Contributor

oh that's bad! looking into it

@mitschabaude mitschabaude added the bug Something isn't working label Feb 20, 2024
@mitschabaude
Copy link
Contributor

started to debug this, some notes:

you can get rid of some of the warnings by importing every type from o1js (not claiming that this is a solution, just mentioning for further investigation)

import type * as o1js from "o1js";

however, what persists are warnings about field.js and bool.js, and I haven't figured out yet exactly how to make them disappear.
image

When looking at the generated d.ts files, you'll find stuff like import('field.js').FieldVar and import('field.js').Field all over the place.

So there might be two problems:

  • The fact that we don't export FieldVar and FieldConst
  • The weird type override we do in core.ts to get Field to be a function and class and type at the same time

@mitschabaude
Copy link
Contributor

Btw, I don't get what exactly makes "nodenext" behave differently here. Types should be (and seem to be) imported from node_modules/o1js/dist/node/index.d.ts no matter the module resolution config

@mitschabaude
Copy link
Contributor

Another long-shot idea is that the cycle of imports between bool.ts and field.ts could be a source of problems since

TypeScript imitates the host’s module resolution, but with types

@mitschabaude
Copy link
Contributor

mitschabaude commented Feb 20, 2024

Ok, I made a lot of progress debugging, will leave this draft PR here for now: #1448

The key change in that PR is to export InternalField and InternalBool from field.ts and bool.ts directly. They can't seem to be resolved to their aliases Field and Bool in "nodenext" mode.

With that PR, the following snippet type-checks. Note the many "unused" type imports, each of which is necessary to "name" the inferred type of HelloProgram

import {
  Bool,
  Field,
  ZkProgram,
  Proof,
  InternalBool,
  InternalField,
  Cache,
  GateType,
  Gate,
  FieldVar,
  FieldConst,
} from "o1js";

export { HelloProgram };

const HelloProgram = ZkProgram({
  name: "hello-program",
  publicInput: Field,
  publicOutput: Bool,
  methods: {
    addition: {
      privateInputs: [Field, Field],
      method(sum: Field, a: Field, b: Field) {
        return a.add(b).equals(sum);
      },
    },
  },
});

alternatively, this could be used:

import { Bool, Field, ZkProgram } from "o1js";
import type * as _o1js from "o1js"; // for type naming

export { HelloProgram };

const HelloProgram = ZkProgram({
  name: "hello-program",
  publicInput: Field,
  publicOutput: Bool,
  methods: {
    addition: {
      privateInputs: [Field, Field],
      method(sum: Field, a: Field, b: Field) {
        return a.add(b).equals(sum);
      },
    },
  },
});

@ukstv
Copy link
Author

ukstv commented Feb 24, 2024

That's not ideal, but it works :) When is the release?

@mitschabaude
Copy link
Contributor

mitschabaude commented Feb 25, 2024

That's not ideal, but it works :) When is the release?

not sure if we want to release that yet

@dfstio
Copy link

dfstio commented Feb 26, 2024

See also the discussion on discord https://discord.com/channels/484437221055922177/1205086733121888266

I've encountered many issues with o1js and nodenext in my MinaNFT library, and after trying many ways to resolve it, I finally decided to wait for TypeScript bug resolution microsoft/TypeScript#47663 and microsoft/TypeScript#48212.

I believe that the bug is on TypeScript side, and only some temporary workarounds can be made in the o1js or minanft library.

@mitschabaude
Copy link
Contributor

We should check if TypeScript 5.5 which includes microsoft/TypeScript#42873 resolves this

@ukstv
Copy link
Author

ukstv commented May 3, 2024

@mitschabaude Nope. Does not work. Now even import type does not work.

Updated the repro repo with the newest TS.

@Trivo25 Trivo25 closed this as completed Oct 14, 2024
@mitschabaude
Copy link
Contributor

I just struggled with this on a new repo again and was forced to use the deprecated module resolution mode there, vote for re-opening

@Trivo25 Trivo25 reopened this Oct 14, 2024
@dfstio
Copy link

dfstio commented Nov 29, 2024

@mitschabaude
I was able to resolve this error by using install-strategy=nested in the .npmrc file with npm and monorepo.

According to the recent discussion on the TypeScript repo, this bug is fixed now in TypeScript, and if we see it, it is a problem with our imports that do not allow TypeScript to understand the type because of types not exported or that are not available due to the location of o1js. Making o1js being referenced as in this error message

src/indexed-map/indexed-map.ts(6,9): error TS2742: The inferred type of 'IndexedMerkleMap' cannot be named without a reference to '../../../../node_modules/o1js/dist/node/lib/provable/merkle-tree-indexed.js'. This is likely not portable. A type annotation is necessary.

cause errors, so in the case of monorepo, installing node_modules directly to the package resolves it.

@mitschabaude
Copy link
Contributor

mitschabaude commented Nov 29, 2024

hm, I don't have a monorepo and get the "cannot be named" error all over the place when I switch to "nodenext" import resolution

(I'm on the latest TS 5.7 beta)

@dfstio
Copy link

dfstio commented Nov 29, 2024

It is my configuration that works for me:
https://github.com/zkcloudworker/minatokens-lib/blob/main/packages/nft/tsconfig.json

I've also tried setting declaration to false and generating declaration files with third-party tools, but all tools give the same error.

I believe that exporting some types from o1js that you use and ensuring that o1js path is acceptable for TypeScript can resolve the error.

Also, o1js should be listed both in devDependencies and peerDependencies

@dfstio
Copy link

dfstio commented Dec 21, 2024

@mitschabaude

hm, I don't have a monorepo and get the "cannot be named" error all over the place when I switch to "nodenext" import resolution

I have added a PR to your repo to convert it to NodeNext. Please check if it works for you.
zksecurity/mina-attestations#93

@dfstio
Copy link

dfstio commented Jan 30, 2025

See also comment on zksecurity/mina-attestations#93 (comment)

@mitschabaude
Copy link
Contributor

mitschabaude commented Jan 30, 2025

Given the comment above, maybe the problem is just that o1js uses baseUrl itself: (I honestly don't remember why)

"baseUrl": ".", // affects where output files end up

EDIT: no I tried removing it, doesn't seem to change anything

@dfstio
Copy link

dfstio commented Jan 30, 2025

Maybe something should be changed that will affect the sourceRoot and sources in the map files that all have empty sourceRoot now:

"sourceRoot":"","sources":["../../../../src/lib/mina/account-update.ts"]

Theoretically, exporting "./dist/node/index.js" and referencing the files above "./dist/node/index.js" should not create problems, but in practice maybe it can affect the paths

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants