Dependency updates; update error message for Libre 2 (UPLOAD-1497, UPLOAD-1500) #1700
Conversation
gniezen
changed the title
There was a problem hiding this comment.
Pull Request Overview
A comprehensive dependency update and error message improvement for Libre 2 device support. The PR updates Node.js runtime, numerous package versions, and refactors module imports to use explicit file extensions while improving user messaging for Libre 2 uploads.
Key changes:
- Node.js version upgrade from 20.14.0 to 22.19.0
- Major package updates including Electron, Babel, and MUI components
- Addition of explicit .js file extensions across module imports
- Enhanced error messaging for Libre 2 uploads with clinic vs personal user distinction
Reviewed Changes
Copilot reviewed 95 out of 96 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Node.js version bump and major dependency updates |
| webpack.config.*.mjs | Entry point updates and alias configuration |
| app/constants/errorMessages.js | Split Libre 2 error messages for clinic vs personal users |
| app/actions/async.js | Logic to display appropriate Libre 2 error message based on user type |
| Multiple .eslintrc files | Disabled import/extensions rule to support new import patterns |
| app/utils/*.js | Converted module exports to ES modules with explicit extensions |
| lib/core/*.js | Added .js extensions to all import statements |
| lib/drivers//.js | Updated import paths with explicit file extensions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
app/main.dev.js
Outdated
| const require = createRequire(import.meta.url); | ||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = path.dirname(__filename); | ||
| const { sync: syncActions } = require('./actions/index.js'); |
There was a problem hiding this comment.
Mixing ES modules (import) and CommonJS (require) patterns in the same file creates inconsistency. Consider converting the CommonJS require to an ES module import or document why this mixed approach is necessary.
| const { sync: syncActions } = require('./actions/index.js'); | |
| import { sync as syncActions } from './actions/index.js'; |
There was a problem hiding this comment.
Yeah, can't just make that suggestion edit. Might be worth leaving a comment about why, though.
There was a problem hiding this comment.
TBH I don't quite understand why it's happening, but if you don't use require you get:
App threw an error during load
SyntaxError: Unexpected token '<'
at compileSourceTextModule (node:internal/modules/esm/utils:347:16)
at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:107:18)
at #translate (node:internal/modules/esm/loader:540:12)
at ModuleLoader.loadAndTranslate (node:internal/modules/esm/loader:587:27)
at async ModuleJob._link (node:internal/modules/esm/module_job:162:19)
A JavaScript error occurred in the main process
Any ideas?
app/utils/uploadDataPeriod.js
Outdated
| import pkg from '../../app/utils/ipc.cjs'; | ||
| const { ipcRenderer, ipcMain } = pkg; | ||
|
|
There was a problem hiding this comment.
The intermediate 'pkg' variable assignment is unnecessary. You can destructure directly from the import: import { ipcRenderer, ipcMain } from '../../app/utils/ipc.cjs';
| import pkg from '../../app/utils/ipc.cjs'; | |
| const { ipcRenderer, ipcMain } = pkg; | |
| import { ipcRenderer, ipcMain } from '../../app/utils/ipc.cjs'; |
There was a problem hiding this comment.
This looks correctly evaluated, but I don't really have an opinion on it one way or another
There was a problem hiding this comment.
Ah yes, changing the extension to .cjs made it unnecessary. I'll change it back.
lib/core/api.js
Outdated
| import { format } from 'util'; | ||
| import sundial from 'sundial'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
| import { v4 as uuidv4 } from 'uuid'; // eslint-disable-line import/no-unresolved |
There was a problem hiding this comment.
The eslint-disable comment suggests an import resolution issue. Since [email protected] is listed in package.json, this should resolve correctly. Consider removing the disable comment or investigating the underlying resolution issue.
| import { v4 as uuidv4 } from 'uuid'; // eslint-disable-line import/no-unresolved | |
| import { v4 as uuidv4 } from 'uuid'; |
lib/core/api.js
Outdated
| import { format } from 'util'; | ||
| import sundial from 'sundial'; | ||
| import { v4 as uuidv4 } from 'uuid'; | ||
| import { v4 as uuidv4 } from 'uuid'; // eslint-disable-line import/no-unresolved |
There was a problem hiding this comment.
AFAICT Copilot is correct? I get no lint issues if I remove the disable.
There was a problem hiding this comment.
Seems to be an eslint config issue - I've found a fix.
krystophv
left a comment
krystophv
left a comment
There was a problem hiding this comment.
LGTM 🚀
I left a handful of comments, mostly about what Copilot called out. Nothing that I think needs addressing but you can take action on them if you feel like it.
Addresses: