fix: backward-compat for ESM etherpad#130
Conversation
- Drop trailing slash on ep_etherpad-lite/node/eejs/ require (index.js) - Replace ep_etherpad-lite/node_modules/async with async (exportWhoDidWhat.js) Backward-compatible with current CJS etherpad release; also compatible with the upcoming ESM etherpad branch which has stricter exports map resolution.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Review Summary by QodoFix ESM compatibility with stricter exports map resolution
WalkthroughsDescription• Remove trailing slash from eejs require path for ESM compatibility • Replace internal node_modules async path with direct dependency • Add async as explicit dependency in package.json • Bump version to 11.0.31 Diagramflowchart LR
A["Require paths<br/>with internal refs"] -->|"Remove trailing slash<br/>& use direct deps"| B["ESM-compatible<br/>require statements"]
C["Implicit async<br/>dependency"] -->|"Add to<br/>package.json"| D["Explicit async<br/>dependency"]
B --> E["Backward-compatible<br/>with CJS & ESM"]
D --> E
File Changes1. exportWhoDidWhat.js
|
Code Review by Qodo
1. No regression test for ESM
|
| 'use strict'; | ||
|
|
||
| const async = require('ep_etherpad-lite/node_modules/async'); | ||
| const async = require('async'); |
There was a problem hiding this comment.
1. No regression test for esm 📘 Rule violation ☼ Reliability
This PR changes module resolution behavior (require('async') and require('.../eejs')) but does
not add or update any regression tests in the same commit. Without a targeted test, the ESM/CJS
compatibility fix can silently regress in future changes.
Agent Prompt
## Issue description
The PR is a bug fix for upcoming ESM Etherpad compatibility, but it does not include a regression test added/updated in the same commit.
## Issue Context
The changes adjust Node module resolution paths:
- `exportWhoDidWhat.js` now uses `require('async')`.
- `index.js` now uses `require('ep_etherpad-lite/node/eejs')` (no trailing slash).
A regression test should be added so future refactors don’t revert to the incompatible require paths.
## Fix Focus Areas
- static/tests/backend/specs/esm_compat_require_paths.js[1-120]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "dependencies": { | ||
| "ep_plugin_helpers": "^0.6.2" | ||
| "ep_plugin_helpers": "^0.6.2", | ||
| "async": "^3.2.6" | ||
| } |
There was a problem hiding this comment.
2. Outdated pnpm lockfile 🐞 Bug ☼ Reliability
package.json adds the new runtime dependency "async" but pnpm-lock.yaml is not updated, so `pnpm i --frozen-lockfile` will fail due to the manifest/lock mismatch. This can break the npm publish workflow and any other installation path that relies on a frozen lockfile.
Agent Prompt
### Issue description
A new dependency (`async`) was added to `package.json`, but `pnpm-lock.yaml` was not regenerated. This causes `pnpm i --frozen-lockfile` to deterministically fail because the lockfile no longer matches the manifest.
### Issue Context
This repo has a publish workflow that explicitly runs `pnpm i --frozen-lockfile`, so a lock mismatch will break the release pipeline.
### Fix
1. Run `pnpm install` (or `pnpm i`) in the repo root to update `pnpm-lock.yaml`.
2. Commit the resulting `pnpm-lock.yaml` changes.
### Fix Focus Areas
- package.json[27-30]
- pnpm-lock.yaml[7-23]
- .github/workflows/npmpublish.yml[70-73]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This PR makes the plugin backward-compatible with the upcoming ESM etherpad branch (ether/etherpad#7605).
Changes:
require("ep_etherpad-lite/node/eejs/")→require("ep_etherpad-lite/node/eejs")(inindex.js)require("ep_etherpad-lite/node_modules/async")withrequire("async")(inexportWhoDidWhat.js)Both changes are backward-compatible with the current CJS etherpad release.