Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the app to ESM modules, replacing CommonJS require/module.exports with import/export, updating IPC channels via contextBridge, and loading renderer scripts as ES modules.
- Added
<script type="module">tags and new*-renderer.jsfiles for ESM-based preload/renderer logic - Converted core files (
main.js,breaksPlanner.js, utilities) from CommonJS to ESM syntax - Updated IPC handlers, channel names, and contextBridge exposure in preload scripts
Reviewed Changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/preferences.html | Added module-based renderer script |
| app/preferences-renderer.js | New ESM renderer, dark-mode image handling logic |
| app/main.js | Converted to ESM imports/exports, refactored IPC |
| app/platform.js | Updated to use window.process APIs |
| app/microbreak-renderer.js | ESM-based microbreak renderer |
| app/break-renderer.js | ESM-based long-break renderer |
| const imagesWithDarkVersion = document.querySelectorAll('[data-has-dark-version]') | ||
| imagesWithDarkVersion.forEach(image => { | ||
| // replace last occurance https://github.com/electron-userland/electron-builder/issues/5152 | ||
| const newSource = image.src.replace(/.([^.]*)$/, '-dark.' + '$1') |
There was a problem hiding this comment.
The regex uses . (any character) instead of \. to match a literal dot. It should be /\.([^.]*)$/ to correctly replace the file extension.
| const newSource = image.src.replace(/.([^.]*)$/, '-dark.' + '$1') | |
| const newSource = image.src.replace(/\.([^.]*)$/, '-dark.' + '$1') |
| const imagesWithDarkVersion = document.querySelectorAll('[data-has-dark-version]') | ||
| if (event.matches) { | ||
| imagesWithDarkVersion.forEach(image => { | ||
| const newSource = image.src.replace(/.([^.]*)$/, '-dark.' + '$1') |
There was a problem hiding this comment.
Same issue here: escape the dot in the regex (/\.([^.]*)$/) to avoid matching unexpected characters before the extension.
| const newSource = image.src.replace(/.([^.]*)$/, '-dark.' + '$1') | |
| const newSource = image.src.replace(/\.([^.]*)$/, '-dark.' + '$1') |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #1599 +/- ##
==========================================
- Coverage 14.94% 13.48% -1.47%
==========================================
Files 30 34 +4
Lines 1974 1958 -16
==========================================
- Hits 295 264 -31
- Misses 1679 1694 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the codebase from CommonJS to ES modules, updating imports/exports, tests, preload scripts, and package metadata to fully adopt ESM.
- Convert
require/module.exportstoimport/exportacross all.jsfiles and update preload scripts - Update tests to supply new dependencies (
humanize-duration,semver) and switch built‐in imports to thenode:specifier - Update
package.jsonto"type": "module", bump Node engine, and refresh dependencies
Reviewed Changes
Copilot reviewed 111 out of 112 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils.js | Updated tests to pass humanize-duration & semver to formatters |
| app/utils/utils.js | Converted helpers to ESM, adjusted signatures, exports |
| app/utils/htmlTranslate.js | Made translate async and removed @electron/remote |
| app/main.js | Switched to ESM imports, revamped IPC handlers & channels |
| package.json | Set ESM type, bumped Node version & deps |
| app/utils/context-bridge-exposers.js | Consolidated and updated context-bridge exposers for ESM |
Comments suppressed due to low confidence (6)
test/utils.js:71
- [nitpick] The description 'Others' is ambiguous. Rename this block to clarify what is being tested, for example
describe('shouldShowNotificationTitle').
describe('Others', () => {
app/utils/utils.js:71
- The
shouldShowNotificationTitlefunction is defined but not exported here. This will break consumers (e.g., the context bridge). Please includeshouldShowNotificationTitlein the export.
}
app/main.js:205
EventEmitter.setMaxListenersis not a valid static method. To change the default max listeners, setEventEmitter.defaultMaxListeners = 200or callsetMaxListenerson a specific emitter instance.
EventEmitter.setMaxListeners(200) // for watching Store changes
app/utils/htmlTranslate.js:7
- [nitpick] Using an async callback inside
forEachwill not be awaited by the outer method. Consider using afor...ofloop toawaiteach translation in sequence orPromise.allon a.map.
this.document.querySelectorAll('[data-i18next]').forEach(async function (element) {
package.json:8
- [nitpick] Pinning the Node engine to a single version can introduce install issues. Consider specifying a range (e.g., ">=22.17.0") unless an exact version is strictly required.
"node": "22.17.0"
app/breaksPlanner.js:2
- [nitpick] Imports of core modules are using
'events'here but'node:events'elsewhere. For consistency, consider using thenode:specifier for all built-in imports.
import EventEmitter from 'events'
|
@hovancik Any updates on when this refactoring will be finished? Since this looks like a major rewrite do you plan to release a new major version? |
|
@devtobi it changes a lot of things, but it's mostly migration from require to import, related changes and some things I catched in between related to new electron version and such. So user will not see any changes. (hopefully) I am pretty happy with current state, so I think I will merge it in coming days and see. I am trying to run |
No description provided.