Skip to content

feat(pic)!: require @icp-sdk/core v6 - #291

Open
marc0olo wants to merge 6 commits into
mainfrom
chore/bump-icp-sdk-core-v6
Open

marc0olo wants to merge 6 commits into
mainfrom
chore/bump-icp-sdk-core-v6

Conversation

@marc0olo

@marc0olo marc0olo commented Sep 21, 2026

Copy link
Copy Markdown
Member

Bumps @icp-sdk/core from v5 to v6 and @icp-sdk/bindgen from 0.2.3 to 0.4.0. The bindgen bump only changes the generator version in the declarations header.

Breaking change

@dfinity/pic declares @icp-sdk/core as a runtime dependency and Principal/Identity cross its public API, so consumers on core v5 resolve a second, incompatible copy and instanceof checks fail across the boundary. The minimum NodeJS also moves to 22.12, because core v6 reaches the ESM-only @noble packages from its CommonJS build and so needs require(esm):

NodeJS require('@dfinity/pic')
20.17.0 fails — require() of ES Module ... not supported
22.9.0 fails
22.18.0 works
24.11.1 works

The published package declared no engines at all, so this adds engines.node >= 22.12. NodeJS 20 is EOL, but 22.x below 22.12 is still in the wild, so this needs to be an explicit signal rather than a silent break.

Jest consumers must update their config

core v6 moves @noble/hashes and @noble/curves to v2, which ship ESM only. Jest resolves core's CJS build, which require()s them, and 8 of 11 suites here failed to load until transformIgnorePatterns was widened. The pattern must match those packages at any depth — npm and Yarn nest them under the dependent when another package pins an incompatible version, a shape pnpm never produces — and must repeat Jest's own .pnp default, which a user-supplied array otherwise replaces.

The Jest guide documents the required config alongside the error message. Vitest loads ESM natively and needs no equivalent setting.

Verified

  • pnpm build, pnpm test:pic (11 suites, 65 tests), pnpm test:examples (8 examples), pnpm format:check — clean
  • scripts/smoke-test-install.sh — npm, pnpm, yarn, bun
  • require('@dfinity/pic') on NodeJS 20.17 / 22.9 / 22.18 / 24.11
  • npm consumer with @noble/hashes v1 hoisted, forcing v2 to nest — confirms the depth-independent pattern

Not included

@icp-sdk/core stays a regular dependency rather than a peerDependency, which would be the stricter way to prevent duplicate copies; that is a separate design decision. Other dependency updates, including vite 8 and vitest 5, are out of scope so the breaking change stays reviewable on its own.

BREAKING CHANGE: @dfinity/pic now requires @icp-sdk/core v6 and NodeJS >= 22.12. Consumers must upgrade core, or Principal and Identity values will cross the PicJS API from a second, incompatible copy. Consumers testing with Jest must also widen transformIgnorePatterns so that @noble/@scure are transformed — see the Jest guide for the required config.

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings September 21, 2026 16:41
@marc0olo
marc0olo requested a review from a team as a code owner September 21, 2026 16:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Update the Bun lockfile, support pnpm encoded paths in Jest patterns, and document or enforce the Node.js minimum version.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Updates PicJS to @icp-sdk/core v6 and @icp-sdk/bindgen v0.4.0, with Jest ESM compatibility changes.

Changes:

  • Upgrades SDK dependencies and lockfiles.
  • Regenerates bindgen declaration headers.
  • Adds Jest transforms and documentation for ESM dependencies.
File Description
pnpm-lock.yaml Updates dependency resolutions.
packages/​pic/​tests/​test-canister/​declarations/​test_canister.did.js Updates generated bindgen metadata.
packages/​pic/​tests/​test-canister/​declarations/​test_canister.did.d.ts Updates generated bindgen metadata.
packages/​pic/​package.json Updates runtime SDK dependency.
packages/​pic/​jest.config.ts Configures Jest ESM transforms.
package.json Updates development dependencies.
examples/​todo/​tests/​jest.config.ts Applies Jest compatibility configuration.
examples/​nns_proxy/​tests/​jest.config.ts Applies Jest compatibility configuration.
examples/​multicanister/​tests/​jest.config.ts Applies Jest compatibility configuration.
examples/​icp_features/​tests/​jest.config.ts Applies Jest compatibility configuration.
examples/​google_search/​tests/​jest.config.ts Applies Jest compatibility configuration.
examples/​clock/​tests/​jest.config.ts Applies Jest compatibility configuration.
docs/​src/​content/​docs/​guides/​using-jest.mdx Documents the Jest configuration.
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/pic/package.json Outdated
@marc0olo
marc0olo force-pushed the chore/bump-icp-sdk-core-v6 branch from bdd28ba to 2712968 Compare September 22, 2026 10:30
@marc0olo

Copy link
Copy Markdown
Member Author

Rebased onto main now that #284#288 are in. Addressing the Copilot review:

  • bun.lock inconsistent — resolved by the rebase. chore: patch dependency advisories and drop bun.lock #284 deleted bun.lock and test: verify the published package installs and runs #286 retired the bun CI matrix, so there is no second dependency graph left to update. The four consumer_install legs (npm, pnpm, yarn, bun) all pass.

  • NodeJS minimum version — valid, and now fixed. core v6 reaches ESM-only @noble packages from its CommonJS build, so loading @dfinity/pic needs require(esm), unflagged in NodeJS 22.12. Verified across versions:

    NodeJS require('@dfinity/pic')
    20.17.0 fails — require() of ES Module ... not supported
    22.9.0 fails
    22.18.0 works
    24.11.1 works

    The published package declared no engines at all, so consumers got no signal. Added engines.node >= 22.12 and documented it in the getting started guide.

  • pnpm encoded paths in the Jest pattern — already handled. pnpm's virtual store encodes as node_modules/.pnpm/@noble+hashes@2.4.0/node_modules/@noble/hashes/…; the (\.pnpm/)? branch matches the first node_modules/ segment, so those files are transformed rather than ignored. This is exercised by the 11 unit suites and the 6 Jest examples, which fail without it, and the same pattern works on the flat npm, yarn and bun layouts.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

No unresolved review issues were identified, and the required compatibility and documentation changes are covered.

Review effort: Lite
Findings: 1 High severity

Open (1)
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Jest allowlists do not support pnpm’s encoded virtual-store paths.

Review effort: Lite
Findings: None

Resolved since last review (1)
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file

Bumps @icp-sdk/core from v5 to v6 and @icp-sdk/bindgen from 0.2.3 to 0.4.0.

core v6 moves @noble/hashes and @noble/curves to v2, which ship ESM only, and
core reaches them from its CommonJS build. That has two consequences.

Jest resolves that CJS build and has no require(esm) support, so the noble and
scure packages have to be transformed instead of ignored along with the rest of
node_modules. The pattern matches them at any depth, because package managers
place them differently: pnpm resolves them inside its virtual store, while npm
and Yarn nest them under the dependent when another package pins an
incompatible version. Verified against an npm consumer holding @noble/hashes v1
at the top level, which nests v2 under @icp-sdk/core. Vitest loads ESM natively
and is unaffected.

NodeJS needs require(esm), unflagged in 22.12. Verified that loading
@dfinity/pic fails on 20.17 and 22.11 and works on 22.12 and 24.11. The package
declared no engines at all, so it now declares engines.node >=22.12 and the
getting started guide states the requirement.

BREAKING CHANGE: `@dfinity/pic` now requires `@icp-sdk/core` v6 and NodeJS >= 22.12. Consumers must upgrade core, or `Principal` and `Identity` values will cross the PicJS API from a second, incompatible copy. Consumers testing with Jest must also add `transformIgnorePatterns: ['node_modules/(?!.*(@noble|@Scure))']` to their Jest config.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@marc0olo
marc0olo force-pushed the chore/bump-icp-sdk-core-v6 branch from 688c67f to e383f5f Compare September 22, 2026 11:50
@marc0olo

Copy link
Copy Markdown
Member Author

Correction on the third Copilot point — I dismissed it too quickly earlier, and it was right.

I claimed the (\.pnpm/)? branch handled pnpm's virtual store and that was sufficient. It is not. That pattern only excludes @noble/@scure when they sit directly under node_modules/ or directly under .pnpm/. pnpm happens to resolve them to exactly that shape, which is why every suite here passed — the repo could not have caught this.

npm and Yarn nest them under the dependent when another package pins an incompatible version:

node_modules/@icp-sdk/core/node_modules/@noble/hashes/sha2.js

The narrow pattern ignores that path, so the config this PR documents would have failed for those consumers with the exact error the guide tells them it prevents.

Reproduced end to end with an npm consumer holding @noble/hashes v1 at the top level, which forces v2 to nest under @icp-sdk/core:

pattern nested npm layout
node_modules/(?!(\.pnpm/)?(@noble|@scure)) fails
node_modules/(?!.*(@noble|@scure)) passes

Switched to the depth-independent pattern in all seven Jest configs and in using-jest.mdx, with the rationale documented so it does not get narrowed again. All suites still pass. History squashed to one commit so the BREAKING CHANGE: footer carries the corrected pattern rather than the broken one.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review effort: Lite
Findings: None

Files not reviewed (1)
  • pnpm-lock.yaml: Generated file

@marc0olo
marc0olo requested a review from lwshang September 22, 2026 12:12
Supplying transformIgnorePatterns replaces Jest's default array rather
than extending it, so the override dropped the `\.pnp\.[^\\/]+$` entry
and Yarn PnP consumers would feed .pnp.cjs through swc.

Also documents the core v6 requirement where consumers will see it, and
fixes the only root-relative docs link, which 404s under the versioned
docs base path.
@marc0olo

Copy link
Copy Markdown
Member Author

Note for whoever merges: this repo squash-merges with squash_merge_commit_message: PR_BODY, so the description becomes the commit message and is the only thing commitizen reads. Keep the BREAKING CHANGE: line at the bottom of the description. Without it the release is still 0.24.0 (major_version_zero: true makes a breaking change and a feat produce the same version), but the changelog would carry no warning.

Kept here rather than in the description so the note itself stays out of the commit message.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Two documentation updates remain to state the package’s actual @icp-sdk/core minimum version.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Low severity

Open (1)
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file

Comment thread docs/src/content/docs/guides/getting-started.mdx Outdated
marc0olo and others added 2 commits September 22, 2026 16:17
packages/pic requires ^6.1.0, but the upgrade guide told consumers to install
^6. Resolving to 6.0.x leaves the project below that floor, so a second nested
copy of core is installed for @dfinity/pic and Principal and Identity fail
instanceof across the two copies — the failure the section warns about.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ^6.1.0 floor was tighter than the code requires. pic imports only
Principal, Identity, AnonymousIdentity, Ed25519KeyIdentity and IDL, all present
in 6.0.0, and the suite passes against it: tsc, 65 unit tests, and the counter,
todo, icp_features, multicanister and google_search example suites.

A floor above a consumer's version is what causes core to be installed a second
time under @dfinity/pic, so the two copies fail instanceof across the API
boundary. Accepting any v6 removes that for the whole major and lets the guide
state the same range the package declares.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@marc0olo

Copy link
Copy Markdown
Member Author

Widened the core range to ^6 in 854cf0e, replacing the ^6.1 docs fix from f4628ad.

The ^6.1.0 floor turned out to be tighter than the code needs — pic imports only Principal, Identity, AnonymousIdentity, Ed25519KeyIdentity and IDL, all present in 6.0.0. Verified by pinning core to 6.0.0 and running:

tsc build         clean
unit tests        65/65
counter (vitest)  14/14
todo (jest)       16/16
icp_features       1/1
multicanister      2/2
google_search      1/1

Since the nested-copy failure only happens when a consumer's version cannot satisfy pic's range, accepting any v6 removes it for the whole major. It also means the guide and the manifest now state the same range, so they can't drift apart — which is what the original comment was about.

Resolution is unchanged: ^6 still installs 6.1.0 today, the latest in the line.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Align the runtime dependency and related guidance with the tested @icp-sdk/core ^6.1.0 minimum.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)
Resolved since last review (1)
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Previously missed (1)

In code that hasn't changed since last review

Low severity Require core v6.1+ to prevent duplicate-instance failures

packages/​pic/​README.md:19

This new guidance still says any v6 is sufficient, so it directs users to a range that can install core 6.0.x even though the package release is aligned with the 6.1.x minimum. That can recreate the duplicate-copy instanceof failure described here; update this sentence to require v6.1 or later.

Comment thread packages/pic/package.json
Comment thread docs/src/content/docs/guides/getting-started.mdx
The root devDependency still read ^6.1.0 after packages/pic moved to ^6. The
examples do not declare core themselves, so they resolve it from the root, and
the two ranges disagreeing meant the workspace tested a narrower range than the
package claims to support.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Update the README’s Node.js documentation link, which still points to the unsupported latest-v20.x documentation.

Review effort: Lite
Findings: None

Resolved since last review (2)
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Previously missed (1)

In code that hasn't changed since last review

Low severity Update stale Node.js v20 documentation links

packages/​pic/​README.md:15

This new minimum is inconsistent with the README's example guidance below, which still links the Node test-runner docs to latest-v20.x (and the same stale link remains in examples/README.md and docs/src/content/docs/guides/more-examples.mdx). With PicJS now requiring Node 22.12+, update those links so users are not directed to documentation for an unsupported runtime.

The link targeted the Node 20 documentation, which is end of life, while the
repository requires Node ^24. The unversioned URL tracks the current release
and cannot go stale again. Updated in all three copies of the sentence.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@marc0olo

Copy link
Copy Markdown
Member Author

Updated the Node.js link in 109917f — but pointed it at the test runner docs rather than the download page.

That sentence lists test runners, and the sibling links go to bun's test CLI docs and Mocha's docs, so a download page would break the parallel. The unversioned URL also can't go stale the way the pinned one did:

before  https://nodejs.org/dist/latest-v20.x/docs/api/test.html   EOL Node 20, repo requires ^24
after   https://nodejs.org/api/test.html                          tracks the current release

The same sentence is duplicated in packages/pic/README.md, examples/README.md and docs/guides/more-examples.mdx, so all three are updated — leaving two pointing at end-of-life docs seemed worse than the slightly wider diff.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants