chore(repo): add ESM-only upgrade playbook #1934
Conversation
|
@CharlieHelps read the instructions in |
|
Thanks—read
Details
|
|
@CharlieHelps CI is failing due to linting errors; specifically prettier wasn't run and is throwing an error about bad formatting. Run linting/formatting scripts and commit the changes. Then update the pull-request.md in instructions to include always running linting and formatting scripts. |
Expand this to see my work.
|
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
- The
filesarray example uses!dist/**/*.map, whichpackage.jsonfilesdoes not support; advise removing negation and disabling map emission instead, or publish maps. Also avoid conflicting withdeclarationMap: true. - The
exportssnippet should prefer an explicit"."entry for broader tooling compatibility; adjust the verificationjqcommand accordingly. - The tsconfig example says to extend the shared plugin config but extends the base config; use
tsconfig.plugin.jsonto inherit plugin-specific settings. - The
preparescript is not cross‑platform; recommendprepackor a Node-based conditional for portability.
Additional notes (1)
- Maintainability |
.charlie/playbooks/upgrade-plugin-to-esm-only.md:98-103
The rollback section lists bothgit checkout -- $PKGandgit restore -SW $PKG, which are redundant. Prefer a single modern command for clarity.
Summary of changes
- Added a new repository playbook:
.charlie/playbooks/upgrade-plugin-to-esm-only.md. - The playbook provides step-by-step guidance to convert a single plugin in
packages/<name>to ESM-only, coveringpackage.jsonfields, TypeScript config, build scripts, source updates, tests, cleanup, verification, rollback, and references. - Includes example snippets for
exports,files, build scripts, andtsconfig.json, plus verification commands (jq/node) and rollback instructions.
charliecreates
bot
requested review from
CharlieHelps
and removed request for
CharlieHelps
|
Fixed CI by running Prettier and tightening the playbook’s Markdown structure. Also updated the PR instructions to explicitly include running Prettier before linting.
Changes
Verification# Prettier check: OK
$ pnpm prettier:check
All matched files use Prettier code style!
# Lint (repo scripts)
$ pnpm lint
ESLint: 0 errors, 10 warnings
Re-requested review from @shellscape and @CharlieHelps. |
… Update exports snippet to nest under '.' for better tooling compatibility\n- Adjust verification jq command to match the new exports shape\n\nRefs #1934
- Use explicit . export with types/import (+default) for compatibility - Replace tsconfig JSON example with symlink to .config/tsconfig.plugin.json - Update verify jq path and add symlink check - Clarify package.json files note and prefer .npmignore for maps
charliecreates
bot
force-pushed
the
ai-1933-charlie-esm-upgrade-playbook
branch
from
33decd6 to
e0094fc
Compare
PR Review: chore(repo): add ESM-only upgrade playbook (#1934)OverviewThis PR introduces a new upgrade playbook under Review Summary
Suggestions & Next Steps
Details for Maintainers
Let us know once you've incorporated the above and CI passes. The overall documentation work here is strong and should prove very helpful to future contributors! |
Please note in the playbook to remove
Please resolve this |
…truct to remove '!dist/**/*.map' if present in package.json (negation unsupported)\n- Keep only modern 'git restore -SW ' in rollback section
|
Addressed both items—thanks for the nudge.
Changes pushed in 7038aeb. Where
Re-requested review from @shellscape and @CharlieHelps. |
There was a problem hiding this comment.
Please add verbiage:
- always convert any JS in
srcto TS - never convert JS in
fixturesto TS - always convert JS in
testto TS - always run Ava tests after JS to TS conversion and any package.json change
- only after verifying that Ava tests run without modifications, always convert Ava tests to Vitest
- only after converting Ava tests to Vitest, then always run Vitest tests after
srcchange
Expand this to see my work.
|
… convert JS in src to TS\n- Never convert fixtures to TS\n- Always convert JS in test to TS\n- Run AVA after JS→TS + package.json changes; then convert to Vitest and run Vitest thereafter\n\nRefs #1933
|
Addressed the requested verbiage in the playbook and pushed an update.
What changed (snippets)
I also kept the existing reminder to close Rollup bundles in tests to avoid leaks. Re-requested review from @shellscape and @CharlieHelps. |
There was a problem hiding this comment.
- The scripts block uses a POSIX-only
prepareanddel-cli, which are not portable; recommend Node-based cleanup andprepack/prepublishOnlyto avoid install-time side effects. - Verify section lacks a Windows symlink check and relies on
tree, which may not exist; add a PowerShell check and preferfind/ls -R. - Verification can also assert the
defaultexport mapping since the example includes it. - Minor: prefer namespace import in the built-in example to avoid TypeScript interop confusion without
esModuleInterop.
Additional notes (5)
-
Compatibility |
.charlie/playbooks/upgrade-plugin-to-esm-only.md:103-106
Thetreecommand is not universally available (not present on macOS by default and often missing on CI images). Prefer a portable alternative so the verification step works for all contributors. -
Maintainability |
.charlie/playbooks/upgrade-plugin-to-esm-only.md:112-114
Since the exports example includes adefaultcondition, it’s worth verifying it alongsidetypesandimportin the jq check for completeness. -
Compatibility |
.charlie/playbooks/upgrade-plugin-to-esm-only.md:85-86
Minor: The example for Node built-ins uses a default import (import path from 'node:path'). To avoid confusion in TypeScript projects that don’t enableesModuleInterop, a namespace import is the most universally compatible form for CJS-based built-ins. -
Readability |
.charlie/playbooks/upgrade-plugin-to-esm-only.md:72-78
On Windows, creating symlinks can fail without elevated privileges or Developer Mode. A brief note here prevents frustration for contributors following the PowerShell steps. -
Readability |
.charlie/playbooks/upgrade-plugin-to-esm-only.md:111-114
You export adefaultcondition in theexports["."]map, but the verification only checkstypesandimport. Includingdefaultin the check ensures the doc and verify steps stay in sync. Also,jqisn’t commonly installed on Windows—providing a Node-based alternative would improve portability.
Summary of changes
- Added a new playbook:
.charlie/playbooks/upgrade-plugin-to-esm-only.md. - Guides converting a single package under
packages/<name>to ESM-only with TypeScript emit todist/. - Covers
package.jsonchanges (explicit"."export withtypes/import/default, engines/peer deps, files), build scripts, TypeScript config via symlink to shared plugin tsconfig, ESM source updates (includingnode:specifiers), tests cleanup, verification steps, rollback, and references. - Includes verification commands (jq, dynamic import) and notes about excluding source maps via
.npmignoreor a package-localtsconfig.build.jsonoverride.
| - Runtime smoke (Node ESM import works): | ||
| ```bash | ||
| node -e "import('file://$PWD/$PKG/dist/index.js').then(() => console.log('ok'))" | ||
| ``` |
There was a problem hiding this comment.
The runtime smoke test uses $PWD and a Bash‑style expansion that’s not Windows‑friendly. A path‑safe Node snippet avoids quoting issues across shells and OSes.
Suggestion
Replace the command with a cross‑platform Node one‑liner that resolves the file path safely:
node -e "const {resolve}=require('node:path');const {pathToFileURL}=require('node:url');import(pathToFileURL(resolve(process.argv[1]))).then(()=>console.log('ok'))" "$PKG/dist/index.js"Reply with "@CharlieHelps yes please" and I’ll update the Verify snippet accordingly.
|
Run failed I ran into an issue and wasn't able to complete the full check. Here’s what I got through:
Blockers you’ll see:
These need to be resolved for the PR to reach a mergeable state. Would you like clarification on the last review comments, or a status check on a particular file or step? You can see my work on the |
…s\n\n- Node >=20.11 supports import.meta.dirname and import.meta.filename; prefer them\n- Keep URL utilities as a fallback for non-file URLs or special cases
| Note: `package.json` `files` does not support negation patterns. To exclude maps from the published package, add an `.npmignore` entry: | ||
|
|
||
| ``` | ||
| dist/**/*.map |
There was a problem hiding this comment.
@CharlieHelps packages should never omit *.map from "files" period.
There was a problem hiding this comment.
Expand this to see my work.
- Submitted a review on PR #1934 noting in
.charlie/playbooks/upgrade-plugin-to-esm-only.mdthatfilesmust include*.mapand source maps must be published. - Pinpointed the conflicting guidance in
.charlie/playbooks/upgrade-plugin-to-esm-only.mdnear thefilesblock (lines ~41–49) that suggests excludingdist/**/*.mapvia.npmignoreand disablingsourceMap/declarationMap. - Drafted corrected guidance: always publish maps; remove
!dist/**/*.mapfromfiles; do not excludedist/**/*.mapin.npmignore; avoid disablingsourceMap/declarationMapfor published packages. - Edited
.charlie/playbooks/upgrade-plugin-to-esm-only.mdto replace map-exclusion advice with: "Always publish source maps. Do not excludedist/**/*.mapvia.npmignore, and do not disablesourceMap/declarationMap"; also clarified thatfilesnegation is not supported. - Ran Prettier to format the repository;
.charlie/playbooks/upgrade-plugin-to-esm-only.mdis formatted and saved cleanly. - Verified working tree with
git status --porcelain: 1 modified file (.charlie/playbooks/upgrade-plugin-to-esm-only.md).
There was a problem hiding this comment.
Addressed — updated the playbook to always publish source maps and removed the guidance to exclude .map files or disable map emission. Commit: ca2ba63.
Changes
- .charlie/playbooks/upgrade-plugin-to-esm-only.md
- Replaced the “exclude maps via .npmignore / disable maps in tsconfig” advice with a clear policy: always publish source maps.
- Kept the note to remove any existing
"!dist/**/*.map"entry sincefilesnegation isn’t supported.
Verification
# Prettier: OK
$ pnpm prettier:check
All matched files use Prettier code style!
# ESLint: 0 errors, 10 warnings
$ pnpm lintRe-requested review from @shellscape and @CharlieHelps.
There was a problem hiding this comment.
- The build scripts use non-portable commands (
del-cli, POSIX-onlyprepare), which can break on Windows; prefer Node-based cleanup andprepack/prepublishOnly. - The example
import path from 'node:path'will fail under Node ESM; use a namespace import (import * as path from 'node:path'). - Verification relies on
tree,jq, and Bash-style expansions, which aren’t cross-platform; provide PowerShell/Node alternatives and also verify thedefaultexport mapping. - Symlink creation/verification lacks Windows-specific guidance and checks; add a brief note about permissions/Developer Mode and a PowerShell validation snippet.
Additional notes (1)
- Readability |
.charlie/playbooks/upgrade-plugin-to-esm-only.md:72-79
Windows symlink creation can fail without elevated privileges or Developer Mode. Adding a brief note will prevent confusion for contributors following the PowerShell steps.
Summary of changes
- Added a new repository playbook:
.charlie/playbooks/upgrade-plugin-to-esm-only.md. - Guides converting a single
packages/<name>plugin to ESM-only: updatespackage.json(explicit"."export withtypes/import/default, engines, peer deps, files), tsc-only build scripts, tsconfig symlink to shared plugin config, ESM source updates (Node ≥20.11import.meta.dirname/filename,node:specifiers), JS→TS conversion rules, AVA→Vitest order of operations, cleanup, verification, and rollback. - Includes verification commands (
tree,jq, dynamic import) and rollback instructions. - Notes
.npmignorefor excluding source maps and removing unsupported negation infilesarrays.
| ```bash | ||
| pnpm -C $PKG build | ||
| tree $PKG/dist | sed -n '1,80p' | ||
| ``` |
There was a problem hiding this comment.
tree is not universally available (missing on macOS by default and many CI images). The verify step should use a more portable alternative (POSIX and PowerShell options).
Suggestion
Suggest portable alternatives:
- POSIX:
find "$PKG/dist" -maxdepth 2 -type f | sed -n '1,80p'- Windows PowerShell (assuming
$pkg = 'packages/<name>'as in the symlink step):
Get-ChildItem -Recurse -Depth 2 "$pkg/dist" | Select-Object -First 80Reply with "@CharlieHelps yes please" if you’d like me to update the Verify section with these alternatives.
| - Type declarations resolve for consumers: | ||
| ```bash | ||
| jq -r '.types, .exports["."].types, .exports["."].import' $PKG/package.json | ||
| ``` |
There was a problem hiding this comment.
The verify step checks types and import but not default, even though the exports["."] example includes it. Also, jq isn’t common on Windows; a Node alternative would improve portability.
Suggestion
Expand the check and add a Node-based alternative:
- jq (POSIX):
jq -r '.types, .exports["."].types, .exports["."].import, .exports["."].default' "$PKG/package.json"- Node (cross-platform):
node -e "const fs=require('node:fs');const pkg=JSON.parse(fs.readFileSync(process.argv[1],'utf8'));console.log(pkg.types,pkg.exports?.['.']?.types,pkg.exports?.['.']?.import,pkg.exports?.['.']?.default)" "$PKG/package.json"Reply with "@CharlieHelps yes please" if you want me to apply these updates.
…ap guidance - Remove suggestion to exclude dist/**/*.map via .npmignore - Remove suggestion to disable sourceMap/declarationMap via tsconfig - Add explicit note to keep maps published and remove any existing exclusions Refs #1934
There was a problem hiding this comment.
- Non-portable commands in the scripts and verification steps (
preparewith POSIX test,del-cli,tree,jq,$PWD) will fail on Windows; prefer Node-based or PowerShell-friendly alternatives andprepack/prepublishOnlyoverprepare. - The Node built-in example uses a default import; prefer a namespace import to avoid ESM/CJS interop confusion in TS without
esModuleInterop. - Verification omits checking the
defaultexport mapping despite documenting it; include it and provide a Node-based check. - Windows-specific guidance is incomplete: add PowerShell variable usage up front, a symlink verification snippet, and a brief note about symlink permissions/Developer Mode.
Summary of changes
- Introduces a new playbook:
.charlie/playbooks/upgrade-plugin-to-esm-only.md. - Guides converting a single
packages/<name>plugin to ESM-only with TypeScript emit todist/. - Covers
package.json(explicit"."export withtypes/import/default, engines, peer deps, files), tsc-only build, tsconfig symlink to shared plugin config, ESM source updates (Node ≥20.11import.meta.dirname/filename,node:specifiers), JS→TS/fixtures rules, AVA→Vitest migration order, cleanup, verification, and rollback. - Verification includes build, symlink check,
jqchecks for types/exports, and a Node ESM runtime smoke test.
| import path from 'node:path'; | ||
|
|
||
| const here = import.meta.dirname; | ||
| // const file = import.meta.filename; | ||
| const pkgJson = path.join(here, 'package.json'); | ||
| ``` |
There was a problem hiding this comment.
Using a default import for Node built-ins can be confusing in TS projects without esModuleInterop and may not reflect common guidance. A namespace import avoids interop pitfalls.
Suggestion
Prefer a namespace import in the example for broad compatibility:
import * as path from 'node:path';
const here = import.meta.dirname;
// const file = import.meta.filename;
const pkgJson = path.join(here, 'package.json');Reply with "@CharlieHelps yes please" if you'd like me to update the snippet accordingly.
Rollup Plugin Name:
repoThis PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
resolves #1933
Description
Adds a scoped playbook under
.charlie/playbooksto guide converting a single plugin package inpackages/<name>to ESM‑only, based on the recent alias migration.Highlights:
packages/<name>).node:specifiers), tests cleanup, and verification steps.Additional notes:
.charlieinstructions:pnpm lint: 0 errors, 10 warningspnpm fix:js: no changes