Skip to content

fix: backward-compat for ESM etherpad#423

Merged
SamTV12345 merged 1 commit into
mainfrom
fix/esm-compat
May 25, 2026
Merged

fix: backward-compat for ESM etherpad#423
SamTV12345 merged 1 commit into
mainfrom
fix/esm-compat

Conversation

@SamTV12345
Copy link
Copy Markdown
Member

This PR makes the plugin backward-compatible with the upcoming ESM etherpad branch (ether/etherpad#7605).

Changes:

  1. Remove trailing slash from require("ep_etherpad-lite/node/eejs/")require("ep_etherpad-lite/node/eejs") (in index.js)
  2. Migrate log4js from require("ep_etherpad-lite/node_modules/log4js") to ep_plugin_helpers/logger (in commentManager.js)

Both changes are backward-compatible with the current CJS etherpad release.

- Drop trailing slash on ep_etherpad-lite/node/eejs/ require (index.js)
- Migrate log4js to ep_plugin_helpers/logger (commentManager.js)

Backward-compatible with current CJS etherpad release; also
compatible with the upcoming ESM etherpad branch which has stricter
exports map resolution.
@qodo-code-review
Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Make plugin backward-compatible with ESM etherpad

🐞 Bug fix ✨ Enhancement

Grey Divider

Walkthroughs

Description
• Remove trailing slash from eejs require path for ESM compatibility
• Migrate log4js to ep_plugin_helpers/logger helper
• Update ep_plugin_helpers dependency to ^0.6.7
• Bump version to 11.1.15
Diagram
flowchart LR
  A["Current CJS<br/>Etherpad"] -->|"Backward<br/>Compatible"| B["Plugin v11.1.15"]
  C["Upcoming ESM<br/>Etherpad"] -->|"Now<br/>Compatible"| B
  B -->|"Remove trailing slash<br/>from eejs path"| D["index.js"]
  B -->|"Use ep_plugin_helpers<br/>logger"| E["commentManager.js"]
  B -->|"Update dependencies"| F["package.json"]

Loading

File Changes

1. index.js 🐞 Bug fix +1/-1

Remove trailing slash from eejs require

• Remove trailing slash from require('ep_etherpad-lite/node/eejs/') path
• Change to require('ep_etherpad-lite/node/eejs') for stricter ESM exports resolution

index.js


2. commentManager.js ✨ Enhancement +2/-2

Migrate log4js to ep_plugin_helpers logger

• Replace direct log4js require from node_modules with ep_plugin_helpers/logger
• Update logger initialization from log4js.getLogger() to createLogger()
• Improves compatibility with ESM etherpad and reduces direct dependency coupling

commentManager.js


3. package.json Dependencies +2/-2

Update version and dependencies

• Bump version from 11.1.14 to 11.1.15
• Update ep_plugin_helpers dependency from ^0.6.0 to ^0.6.7

package.json


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented May 25, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1)

Grey Divider


Action required

1. No regression test added 📘 Rule violation ☼ Reliability
Description
This PR changes runtime behavior (logger import and eejs require path) but does not add or update
any regression test to ensure the bugfix remains covered. Without a test that fails pre-fix and
passes post-fix, future refactors could silently reintroduce the compatibility issue.
Code

index.js[7]

Evidence
PR Compliance ID 1 requires that every bug fix include a regression test in the same commit. The
diff shows only production code changes to module imports (notably
require('ep_etherpad-lite/node/eejs') and createLogger('ep_comments_page')) and contains no
accompanying test changes.

AGENTS.md: Every Bug Fix Must Include a Regression Test in the Same Commit
index.js[7-7]
commentManager.js[5-9]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
This PR introduces bugfix/compatibility changes but does not include a regression test added/updated in the same commit.

## Issue Context
The changes affect module resolution at runtime (switching `eejs` require path and migrating logging to `ep_plugin_helpers/logger`). A regression test should fail on the pre-fix code and pass with this PR applied.

## Fix Focus Areas
- index.js[7-7]
- commentManager.js[5-9]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Outdated pnpm lockfile 🐞 Bug ☼ Reliability
Description
package.json now depends on ep_plugin_helpers@^0.6.7 but pnpm-lock.yaml in this PR branch still
resolves ep_plugin_helpers to 0.6.2 (and the old specifier ^0.6.0). This can make pnpm installs fail
under frozen lockfile settings and can leave the code running with an older ep_plugin_helpers while
commentManager.js now requires ep_plugin_helpers/logger.
Code

package.json[26]

Evidence
The PR branch shows a new dependency requirement (^0.6.7) and a new import from
ep_plugin_helpers/logger, but the lockfile still records the old specifier and resolution
(^0.6.0 -> 0.6.2), demonstrating that the lockfile is out of sync with the code and declared
dependencies.

package.json[24-28]
pnpm-lock.yaml[9-22]
pnpm-lock.yaml[508-511]
commentManager.js[5-9]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`package.json` was updated to require `ep_plugin_helpers: ^0.6.7`, and the code now imports `ep_plugin_helpers/logger`, but `pnpm-lock.yaml` is still pinned to `ep_plugin_helpers@0.6.2` (and shows the old specifier `^0.6.0`). This makes installs non-reproducible and commonly breaks CI when using `pnpm install --frozen-lockfile`.

## Issue Context
The updated code path is in `commentManager.js` which now does `require('ep_plugin_helpers/logger')`.

## Fix Focus Areas
- package.json[24-28]
- pnpm-lock.yaml[9-22]
- pnpm-lock.yaml[508-511]
- commentManager.js[5-9]

## Suggested fix
1. Run `pnpm install` (or `pnpm install --lockfile-only`) so the lockfile matches the updated dependency range.
2. Commit the updated `pnpm-lock.yaml` so `ep_plugin_helpers` resolves to a version compatible with the new `ep_plugin_helpers/logger` import.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread index.js
const AttributePool = require('ep_etherpad-lite/static/js/AttributePool').default || require('ep_etherpad-lite/static/js/AttributePool');
const Changeset = require('ep_etherpad-lite/static/js/Changeset').default || require('ep_etherpad-lite/static/js/Changeset');
const eejs = require('ep_etherpad-lite/node/eejs/');
const eejs = require('ep_etherpad-lite/node/eejs');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. No regression test added 📘 Rule violation ☼ Reliability

This PR changes runtime behavior (logger import and eejs require path) but does not add or update
any regression test to ensure the bugfix remains covered. Without a test that fails pre-fix and
passes post-fix, future refactors could silently reintroduce the compatibility issue.
Agent Prompt
## Issue description
This PR introduces bugfix/compatibility changes but does not include a regression test added/updated in the same commit.

## Issue Context
The changes affect module resolution at runtime (switching `eejs` require path and migrating logging to `ep_plugin_helpers/logger`). A regression test should fail on the pre-fix code and pass with this PR applied.

## Fix Focus Areas
- index.js[7-7]
- commentManager.js[5-9]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread package.json
"dependencies": {
"cheerio": "^1.2.0",
"ep_plugin_helpers": "^0.6.0",
"ep_plugin_helpers": "^0.6.7",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

2. Outdated pnpm lockfile 🐞 Bug ☼ Reliability

package.json now depends on ep_plugin_helpers@^0.6.7 but pnpm-lock.yaml in this PR branch still
resolves ep_plugin_helpers to 0.6.2 (and the old specifier ^0.6.0). This can make pnpm installs fail
under frozen lockfile settings and can leave the code running with an older ep_plugin_helpers while
commentManager.js now requires ep_plugin_helpers/logger.
Agent Prompt
## Issue description
`package.json` was updated to require `ep_plugin_helpers: ^0.6.7`, and the code now imports `ep_plugin_helpers/logger`, but `pnpm-lock.yaml` is still pinned to `ep_plugin_helpers@0.6.2` (and shows the old specifier `^0.6.0`). This makes installs non-reproducible and commonly breaks CI when using `pnpm install --frozen-lockfile`.

## Issue Context
The updated code path is in `commentManager.js` which now does `require('ep_plugin_helpers/logger')`.

## Fix Focus Areas
- package.json[24-28]
- pnpm-lock.yaml[9-22]
- pnpm-lock.yaml[508-511]
- commentManager.js[5-9]

## Suggested fix
1. Run `pnpm install` (or `pnpm install --lockfile-only`) so the lockfile matches the updated dependency range.
2. Commit the updated `pnpm-lock.yaml` so `ep_plugin_helpers` resolves to a version compatible with the new `ep_plugin_helpers/logger` import.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@SamTV12345 SamTV12345 merged commit c479477 into main May 25, 2026
7 checks passed
@SamTV12345 SamTV12345 deleted the fix/esm-compat branch May 25, 2026 16:01
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.

1 participant