-
-
Notifications
You must be signed in to change notification settings - Fork 229
chore: move to esm #4403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: move to esm #4403
Conversation
Pull Request Test Coverage Report for Build 18403500992Details
💛 - Coveralls |
AlCalzone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a couple of things.
In addition it might be a good time to ditch fs-extra in favor of node:fs/promises. Only requires two helper functions for deleting and creating directories, but Copilot is pretty good at replacing fs-extra I've noticed.
|
|
||
| const url = require('native-url') | ||
| import { join } from 'node:path' | ||
| import url from 'native-url' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use the browser-compatible WhatWG URL API instead of an extra dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
every time I tried using that I ended up reverting my changes as them are not 100% compatibile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I did this in Z-Wave JS to enable browser support and it worked just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See: mqttjs/MQTT.js#1927
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the URL API would also prevent the need for this patch.
This pull request introduces several significant updates across the codebase, primarily focused on modernizing the Node.js environment, allow code sharing between backend and UI, improving TypeScript usage, and enhancing import consistency. The most notable changes include upgrading to Node.js 22, transitioning all imports to use explicit
.tsextensions and more precise type-only imports, and updating workflow and configuration files to align with these changes.Node.js and Tooling Upgrades:
.github/workflows/test-application.yml) and.nvmrc, ensuring the project uses the latest LTS features and performance improvements. [1] [2]TypeScript and Import Improvements:
.tsextensions and transitioned to type-only imports where appropriate, improving type safety and compatibility with ES module standards. This affects nearly all files in theapi/directory, includingapp.ts,config/store.ts,lib/Gateway.ts, and others. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]Configuration and Linting Updates:
.prettierrc.jsto use ES module export syntax, aligning with modern JavaScript module standards.watch-filesoption from.mocharc.yml, possibly to streamline or fix test runner behavior.Codebase Consistency and Minor Fixes:
node:prefix (e.g.,node:fs,node:path) across the codebase. [1] [2] [3] [4] [5]deviceConfigPriorityDirinConstants.tsand updating its usage inconfig/store.ts. [1] [2]These changes collectively modernize the codebase, improve maintainability, and ensure compatibility with the latest Node.js and TypeScript features.
References:
[1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20]
Fixes #4401