Skip to content

test: trip when the mirrored editor types move upstream - #63

Open
sidgaikwad wants to merge 2 commits into
unlayer:mainfrom
sidgaikwad:test/types-drift-tripwire
Open

test: trip when the mirrored editor types move upstream#63
sidgaikwad wants to merge 2 commits into
unlayer:mainfrom
sidgaikwad:test/types-drift-tripwire

Conversation

@sidgaikwad

Copy link
Copy Markdown
Contributor

Partially addresses #42 — please read the caveat before merging.

What this does

src/types.ts hand-copies MountOptions, ImageEditorInstance, ImageEditorEmbed and ImageEditorSaveResult from the private @unlayer/image-editor package, with a comment saying it does so "until they move into @unlayer/types". Nothing enforced the "until".

Once that move happens, the local copies silently become a second, diverging source of truth. That's exactly how #25 (Features.imageEditor missing dock/corners) reached users.

test/typesDrift.test.ts scans the installed @unlayer/types declarations and fails the moment any mirrored name appears there:

AssertionError: @unlayer/types now exports JSONTemplate. Delete the mirrored
declaration(s) from src/types.ts and re-export from @unlayer/types instead,
then drop the name(s) from this test.

(That output is from deliberately adding a name that does exist upstream, to prove the tripwire fires — it passes as-is today, since @unlayer/types@1.477.0 exports none of the four.)

The KEEP IN SYNC comment now points at the test, so the enforcement is discoverable from the declarations.

What this deliberately does NOT do

It does not detect the private @unlayer/image-editor package changing underneath you — the more dangerous direction, where a renamed instance method typechecks fine and throws at runtime.

Of the three options in the issue:

  1. Move the types into @unlayer/types — the durable fix, and the comment's own stated intent. Only Unlayer can publish that.
  2. CI drift check against the private package — needs access this repository doesn't have.
  3. Runtime shape assertion — I chose not to add one. It would mean loading the live CDN embed in CI, which makes the suite network-dependent and flaky for a check that only fires on a rename.

So this closes the cheap half and leaves the real fix as yours. Happy to do the @unlayer/types migration in a follow-up if you publish the types there.

Tests 47, coverage still 100%; lint, typecheck, build clean.

src/types.ts hand-copies MountOptions, ImageEditorInstance,
ImageEditorEmbed and ImageEditorSaveResult from the private
@unlayer/image-editor package, with a comment saying it does so "until
they move into @unlayer/types". Nothing enforced the "until".

Once the move happens the local copies silently become a second,
diverging source of truth — which is how a missing dock/corners reached
users in unlayer#25.

Add a tripwire that scans the installed @unlayer/types declarations and
fails the moment any mirrored name appears there, with a message saying to
delete the local copy and re-export instead.

This is deliberately not a full drift check. Detecting the private package
changing underneath us needs access this repository does not have, so that
half stays a human responsibility — now at least written down next to the
declarations.
@vercel

vercel Bot commented Sep 4, 2026

Copy link
Copy Markdown

@sidgaikwad is attempting to deploy a commit to the Unlayer Team on Vercel.

A member of the Team first needs to authorize it.

@ivoIturrieta

Copy link
Copy Markdown
Collaborator

Please add the Node type configuration needed by this test. typecheck currently fails on node:fs, node:path, and process

test/typesDrift.test.ts reads the installed @unlayer/types declarations, so
it imports node:fs and node:path and calls process.cwd(). None of that
typechecked: @types/node was never a root dependency, and tsconfig's
"types" array listed only vitest/globals, which excludes the Node globals.

Add @types/node (pinned exact, 24.10.9 — matching the demo's pin) and add
"node" to the types array. Both halves are required: with the package
installed but "node" absent from the array, tsc still reports TS2591 for
all three.

This passed locally for me because a stray @types/node in a parent
directory of my checkout was being picked up by TypeScript's upward
resolution. Verified in a clean tree outside that path.
@sidgaikwad

Copy link
Copy Markdown
Contributor Author

Fixed in 90294bd. Sorry — that should not have reached you.

Why my local typecheck passed: there is a stray @types/node in a parent directory of my checkout (/Users/mac/node_modules), and TypeScript's resolution walks up into it. So node:fs resolved from outside the project entirely. I reproduced your failure in a clean tree outside that path:

probe.ts(1,30): error TS2591: Cannot find name 'node:fs'. ... Try `npm i --save-dev @types/node`
                              and then add 'node' to the types field in your tsconfig.
probe.ts(2,22): error TS2591: Cannot find name 'node:path'. ...
probe.ts(3,36): error TS2591: Cannot find name 'process'. ...

Exactly the three you named.

The fix — both halves are required:

  • @types/node as a root devDependency, pinned exact at 24.10.9 (matching the demo's existing pin, per the CONTRIBUTING rule on exact versions).
  • "node" added to compilerOptions.types, which was ["vitest/globals"] — that array suppresses the Node globals.

Verified in the isolated tree that installing the package alone is not enough: with types unchanged, tsc still reports TS2591 for all three. Only both together give a clean exit 0.

One thing worth a look in the diff. npm install on my machine (npm 10.9.8) silently stripped the libc field from 23 Linux optional packages (@rollup/rollup-linux-*, lightningcss-linux-*, @rolldown/binding-linux-*) — presumably a different npm major than the one that generated the committed lockfile. Since dropping libc can affect musl vs glibc optional-dependency selection on your Linux runners, I restored those fields by hand. The lockfile diff is now +18 lines, 0 deletions: just @types/node and its undici-types dep, plus the root devDependencies entry. npm ci validates clean against it.

47 tests, coverage still 100%, lint/typecheck/build clean.


One judgement call I would like your view on: adding "node" to the top-level types array makes Node globals visible in src/ too, which is browser-only code — so a stray process.env in the component would now typecheck. If you would rather keep that boundary, the alternative is a separate tsconfig for test/. I went with the simpler change since it is what you asked for, but happy to scope it if you prefer.

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