Skip to content

ci(js-package-testing): OIDC npm publish for protocol visualization alphas#22014

Open
y3rsh wants to merge 25 commits into
edgefrom
npm-publish-alpha
Open

ci(js-package-testing): OIDC npm publish for protocol visualization alphas#22014
y3rsh wants to merge 25 commits into
edgefrom
npm-publish-alpha

Conversation

@y3rsh

@y3rsh y3rsh commented Jul 22, 2026

Copy link
Copy Markdown
Member

Overview

Adds a manual GitHub Actions workflow to publish the four @opentrons/* JS packages used for protocol visualization on web products, using npm Trusted Publishing (OIDC) instead of long-lived tokens.

These packages are published as alphas for internal Opentrons use only. They are not a supported public API: they may change or disappear without notice.

Packages

  • @opentrons/shared-data
  • @opentrons/step-generation
  • @opentrons/components
  • @opentrons/protocol-visualization

Acceptance criteria

  • Manual workflow npm-publish-alpha.yaml can publish via OIDC (id-token: write, environment npm-publish)
  • Versioning is automatic: read npm latest, patch-bump to next *-alpha.0, publish with dist-tag latest only
  • scripts/publish.mts packs with pnpm pack (so catalog: / workspace:* are rewritten by pnpm), then publishes with npm publish for OIDC on pnpm 10.x
  • Debt patches that remain (files / exports / @types moves / peer ranges) live in scripts/package-json-patches.mts and are shared with js-package-testing
  • Follow-up when the monorepo moves to pnpm 11+: switch the publish step to pnpm publish (native OIDC)
  • npm Trusted Publisher configured for all four packages (org Opentrons, repo opentrons, workflow npm-publish-alpha.yaml, environment npm-publish)

Test Plan and Hands on Testing

  • Local js-package-testing: make teardown setup test-setup test (Eyes baselines updated; tests pass)
  • workflow_dispatch JS package testing from branch npm-publish-alpha (checkout now uses the selected branch, not hardcoded edge) passed
  • Local dry-run: node --experimental-strip-types scripts/publish.mts --version 0.3.9-alpha.0 --dry-run (from npm latest 0.3.8-alpha.0); all four manifests concrete semver, no catalog:/workspace:*, @opentrons/* deps = 0.3.9-alpha.0
  • Run publish workflow with dry_run: true once the workflow file exists on a Trusted Publisher-compatible ref
  • Approve the npm-publish environment and confirm a real publish lands on npm latest with provenance
  • Confirm scripts/next-npm-version.mts returns 0.3.9-alpha.0; confirm publish.mts --tag is rejected (hardcoded latest)
  • Confirm old token-based publish paths fail as expected (tokens disallowed)

Changelog

  • Add .github/workflows/npm-publish-alpha.yaml (manual dispatch, OIDC, environment npm-publish)
  • Colocate publish helpers under scripts/ (publish.mts, next-npm-version.mts, package-json-patches.mts, etc.)
  • Use pnpm pack for catalog: / workspace:* rewriting; keep npm publish for Trusted Publishing on pnpm 10
  • Shrink package-json-patches.mts to debt-only fixes (files / exports / @types / peer ranges)
  • Fix js-package-testing.yaml so workflow_dispatch checks out the selected branch (cron still pins edge)
  • Remove stale react-redux / react-window / redux from js-package-testing Vite optimizeDeps.include (not host deps; only caused resolve warnings)
  • Always publish dist-tag latest with hardcoded patch-to-*-alpha.0 versioning
  • Docs/skill updates for the pack/publish split and pnpm 11 follow-up

Review requests

  • Does the pnpm pack + npm publish split address the concern about script-layer protocol rewriting?
  • Are the remaining debt patches in package-json-patches.mts acceptable as a temporary layer, with source package.json cleanup as follow-up?
  • Confirm Trusted Publisher + npm-publish environment setup matches the workflow filename and environment name exactly
  • Confirm always-*-alpha.0 + latest matches how internal web consumers should install

Risk assessment

  • Attention level: medium (publishes public npm packages; blast radius limited to alpha internal consumers)
  • Trade-offs: npm publish is required for reliable OIDC on pnpm 10; pnpm publish is deferred to the pnpm 11 upgrade. Remaining debt patches can hide source-config problems if we do not shrink them over time.
  • Blast radius: npm package versions for the four libraries; does not affect robot firmware or App production releases
  • Mitigation: environment required reviewers, dry-run input, tokens disallowed on npmjs, shared patch module with js-package-testing, pnpm pack owns protocol rewriting

y3rsh added 20 commits April 16, 2026 16:20
…consumers

Share package.json patching between publish.mts and local pack setup so
workspace:* and catalog: specifiers resolve like npm installs, and document
required CSS, i18n, and peer dependency bootstrap for external apps.
Publish once under alpha, then add the latest dist-tag so npm install
without a tag resolves the current alpha release.
Add a manual GitHub Action that patch-bumps from npm latest and publishes
the four JS packages with Trusted Publishing (no npm token). Harden
publish.mts to always use the latest dist-tag for a simpler release path.
Keep version resolution with other repo scripts; update workflow and docs.
Keep the npm publish entrypoint with other repo scripts; patch helpers stay
in js-package-testing for the pack/ consumer harness.
CI frozen-lockfile installs failed because package.json dropped the dep
without updating the root lockfile.
Move npm-latest-versions and package-json-patches next to publish.mts so
published-manifest logic has one owner shared by CI publish and pack/.
Keep all npm publish/pack helper scripts under root scripts/.
@y3rsh y3rsh self-assigned this Jul 22, 2026
@y3rsh
y3rsh marked this pull request as ready for review July 22, 2026 16:14

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.

i think we can remove packages that in the root package.json such as typescript, vite etc.
if this one's main purpose is to test the published packages, using a separated repo would be a better choice for us?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes that is an option we can consider, though I do like being able to test on PRs in this repo that changes to the packages don't break their packed form.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agree on keeping this in the monorepo for PR-time pack/smoke testing.

Leaving the local tooling deps (vite, typescript, Playwright, etc.) in this package.json on purpose so js-package-testing looks more like a real external consumer instead of relying on root hoisting. A separate repo is still a future option if we want stricter isolation later; not required to land the OIDC publish path.

| `@opentrons/protocol-visualization` | Annotated protocol steps UI |

```bash
npm install @opentrons/shared-data@X.Y.Z \

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.

not pnpm?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good call. Updated the consumer guide to lead with pnpm add, and kept an npm install example as well.

const major = Number(match[1])
const minor = Number(match[2])
const patch = Number(match[3])
return `${major}.${minor}.${patch + 1}-alpha.0`

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.

will we keep alpha.0 all the time?
what is a merit to do that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, intentionally always *-alpha.0 with a patch bump (0.3.8-alpha.0 -> 0.3.9-alpha.0).

Merit:

  • every published version string clearly signals unsupported / internal-only alpha
  • one simple monotonic line for internal web consumers on npm latest
  • avoids mixing prerelease increments (alpha.1, alpha.2) with patch bumps

Documented that rationale in scripts/next-npm-version.mts.

console.log(`${name}`)
console.log(` latest (dist-tag): ${latest}`)
console.log(` dist-tags: ${formatDistTags(tags)}`)
console.log('')

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.

to make a new line?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, exactly: blank line between packages for readability. Added a short comment above that console.log('') to make the intent obvious.

Lead the consumer install guide with pnpm, document why versions stay
on *-alpha.0, and clarify the blank-line separator in npm-latest-versions.

@SyntaxColoring SyntaxColoring 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.

I haven't looked closely at the implementation (yet), but no objection from me in principle.

Comment thread scripts/publish.mts Outdated
Comment on lines +26 to +38
* What this script fixes vs raw monorepo package.json files:
* 1. workspace:* / link: deps rewritten to real semver so published packages are usable.
* 2. catalog: / catalog:react18 specifiers rewritten to concrete semver ranges.
* 3. @types/* and unused runtime deps moved to devDependencies so consumers
* don't install them.
* 4. peerDependencies rewritten to npm-compatible semver ranges.
* 5. shared-data gets a proper `files` allowlist (lib/ only, no Makefile/Python).
* 6. shared-data exports map verified to point at real CJS + ESM outputs.
* 7. components exports map verified (require -> .js, import -> .mjs).
* 8. protocol-visualization exports include ./styles -> lib/style.css.
* 9. Smoke-tests the ESM output with Node's native dynamic import before publish.
* 10. Each package ships a README (alpha disclaimer) and LICENSE (Apache-2.0
* inherited from the monorepo root).

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.

Is all of this comment actually true? I'm not seeing anything that rewrites peerDependencies or catalogs, for example.

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.

If these are true:

I have seen very frightening things in our JS project configs, and it's not surprising to me that we would need to fix some of them before NPM publishing works properly. But I'm worried about fixing them by adding a new layer of "script fixes," because it might make it harder and more confusing to fix the underlying problems.

For instance, rewriting catalog to concrete semver ranges seems like something that pnpm publish ought to be taking care of. If it's not, then something we did is broken. If I come along later and fix that underlying broken thing...will that break this script? How would I know about it, and test it?

I guess it depends on how much of this comment is actually current, though.

@y3rsh y3rsh Jul 23, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, those rewrites are real; they live in scripts/package-json-patches.mts (imported by publish.mts), not inline in this file. Clarified the header/README so that is obvious.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed - one issue is we cannot use our current pnpm version for trusted publishing, documenting that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed on the risk. We do not want a permanent “script fixes” layer for things pnpm already owns.

What we changed:

  • catalog: / workspace:* rewriting is no longer in package-json-patches.mts. publish.mts temporarily sets the publish version, runs pnpm pack, then restores the source manifests. pnpm does the protocol rewrite.
  • We still use npm publish for the actual upload because Trusted Publishing OIDC is not reliable on our current pnpm 10 pin. When we move the monorepo to pnpm 11+, the plan is to switch that step to pnpm publish and drop the npm publish call.
  • What remains in package-json-patches.mts is intentional debt / consumer hygiene (files, exports, moving @types/*, peer ranges like ^18.2.0). Prefer fixing those in the source package.jsons over time and shrinking the script.

How you’d know / test if an underlying fix makes a patch obsolete:

  • same module is used by js-package-testing pack + publish.mts --dry-run
  • if source manifests become publishable without a given rewrite, delete that rewrite and confirm pack/dry-run still produce a sane consumer manifest

y3rsh added 2 commits July 23, 2026 09:06
Point reviewers at package-json-patches.mts for catalog/workspace/peer
rewrites so the publish.mts header is not misleading.
y3rsh added 2 commits July 23, 2026 09:23
Drop custom catalog/workspace rewriting from package-json-patches; publish
via temporary version bump + pnpm pack, then npm publish for OIDC on pnpm 10.
Document the pnpm 11 switch to pnpm publish, and fix js-package-testing
workflow_dispatch to checkout the selected branch.
Remove react-redux, react-window, and redux from optimizeDeps.include; they
are not host deps and only produced Vite resolve warnings.
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.

3 participants