Skip to content

fix: backward-compat for ESM etherpad#99

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

fix: backward-compat for ESM etherpad#99
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).

Change: Remove trailing slash from require("ep_etherpad-lite/node/eejs/")require("ep_etherpad-lite/node/eejs")

The trailing-slash form breaks under Node's strict ESM exports map resolution. This change is backward-compatible with the current CJS etherpad release.

- Drop trailing slash on ep_etherpad-lite/node/eejs/ require

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

Fix ESM compatibility by removing trailing slash

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Remove trailing slash from eejs require path
• Ensures compatibility with upcoming ESM etherpad
• Maintains backward compatibility with current CJS
• Bump version to 0.1.8
Diagram
flowchart LR
  A["require path with trailing slash"] -- "remove trailing slash" --> B["require path without trailing slash"]
  B -- "compatible with" --> C["ESM etherpad"]
  B -- "compatible with" --> D["CJS etherpad"]

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/') to
 require('ep_etherpad-lite/node/eejs')
• Fixes strict ESM exports map resolution
• Maintains backward compatibility with current CJS release

index.js


2. package.json ⚙️ Configuration changes +1/-1

Update version for ESM compatibility fix

• Bump version from 0.1.7 to 0.1.8

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. Bugfix lacks regression test 📘 Rule violation ☼ Reliability Standing rules for agent edits
Description
This PR changes runtime behavior (ESM compatibility) but does not add or update any regression test
in the same commit. Without a regression test, the trailing-slash resolution issue could be
reintroduced unnoticed.
Code

index.js[5]

Evidence
PR Compliance ID 1 requires a regression test for every bug fix. The diff shows a functional fix to
the eejs import path but no accompanying test change/addition in the PR diff.

Rule Standing rules for agent edits: Every Bug Fix Must Include a Regression Test in the Same Commit
index.js[5-5]

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

## Issue description
A bug fix was made (changing the `eejs` require path to remove the trailing slash) but no regression test was added/updated alongside the fix.

## Issue Context
The compliance rule requires every bug fix to include a regression test in the same commit/PR so the issue is verifiable and cannot silently regress.

## Fix Focus Areas
- index.js[5-5]
- static/tests/frontend-new/specs/smoke.spec.ts[1-13]

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



Remediation recommended

2. Unused eejs import 🐞 Bug ⚙ Maintainability
Description
index.js assigns require('ep_etherpad-lite/node/eejs') to eejs but never references eejs, so
the plugin still performs that deep import at startup even if it's unnecessary. This adds dead code
and can cause avoidable startup failures if that deep import path changes again.
Code

index.js[5]

Evidence
The module is imported into eejs but eejs is never referenced anywhere else in the file, which
ends shortly after the imports and exports declarations.

index.js[3-29]

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

## Issue description
`eejs` is required and assigned to a variable, but the variable is never used in `index.js`. This keeps an unnecessary deep import at startup and makes the file misleading.

## Issue Context
The PR’s goal is ESM-exports compatibility; keeping unused deep imports increases fragility and maintenance cost.

## Fix Focus Areas
- index.js[3-7]

## Suggested fix
- If no side effects are required: remove the `eejs` require entirely.
- If side effects are required: change it to an unassigned side-effect import and add a short comment, e.g.:
 ```js
 // Required for side effects: <explain why>
 require('ep_etherpad-lite/node/eejs');
 ```

ⓘ 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 {template} = require('ep_plugin_helpers');

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. Bugfix lacks regression test 📘 Rule violation ☼ Reliability

Standing rules for agent edits

This PR changes runtime behavior (ESM compatibility) but does not add or update any regression test
in the same commit. Without a regression test, the trailing-slash resolution issue could be
reintroduced unnoticed.
Agent Prompt
## Issue description
A bug fix was made (changing the `eejs` require path to remove the trailing slash) but no regression test was added/updated alongside the fix.

## Issue Context
The compliance rule requires every bug fix to include a regression test in the same commit/PR so the issue is verifiable and cannot silently regress.

## Fix Focus Areas
- index.js[5-5]
- static/tests/frontend-new/specs/smoke.spec.ts[1-13]

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

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