-
Notifications
You must be signed in to change notification settings - Fork 37
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
refactor: convert to ESM + update all dependencies #277
Conversation
BREAKING CHANGE: We now require node 20 or higher
const apiPath = './vendor/electron/docs/api.json' | ||
|
||
const definitionLines = generateDefinitions({ electronApi: require(apiPath) }) | ||
const definitionLines = generateDefinitions({ electronApi: loadJSON(apiPath) }) |
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.
Is the loadJSON
function just a placeholder? Should we made this code copy/pasteable?
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.
The impl of loadJSON
would be as much code as the rest of this doc, so, left as exercise to the reader
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.
My two cents, if we're going to change test frameworks I have a preference for vitest
over jest
. It's faster, works better with ESM, and requires less boilerplate to work with modern codebases (e.g., you need @types/jest
, ts-jest
, cross-env
, --experimental-vm-modules
, etc while vitest
Just Works). Alternatively node:test
could handle the testing needs here, I haven't checked.
@dsanders11 This is just aligning the test framework with docs-parser (jest across the board). My preference with test frameworks is to wait for |
🎉 This PR is included in version 9.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
BREAKING CHANGE: We now require node 20 or higher
Should wait for electron/docs-parser#120