-
-
Notifications
You must be signed in to change notification settings - Fork 399
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Switch to ESM #1293
base: master
Are you sure you want to change the base?
Switch to ESM #1293
Conversation
Excluding the .idea folder helps keep the repository clean from IDE-specific files. This ensures no local IDE settings are unintentionally shared.
I am wondering what can be wrong though |
347f14d
to
c1925dd
Compare
Replaced CommonJS module syntax with ES Modules across the codebase, including imports for dependencies. Updated package.json to declare module type. Made minor syntax adjustments for consistency.
Howdy! I think an ESM-only release is a nonstarter—there are still many things where ESM support is experimental or brittle (like if you’re using jest), which would require people to use an old version of this library (which is already a common source of issues being opened). FWIW, if this library was written in typescript, simultaneous ESM/CJS support would be simpler, but that’s a good chunk of work, and I’m not sure how @JoshuaWise feels about TS. |
for (const str of pragma) db.pragma(str); | ||
return db; | ||
}], | ||
['node-sqlite3', async (filename, pragma) => { | ||
const driver = require('sqlite3').Database; | ||
const db = await (require('sqlite').open)({ filename, driver }); | ||
const db = await (await import('sqlite')).open({ filename, driver: NodeSqlite.Database }); |
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.
No, this is wrong in the benchmark. You are "preloading" the better-sqlite3
outside of its iterations, but then importing sqlite
in every iteration.
|
||
const dest = process.argv[2]; | ||
const source = path.resolve(path.sep, process.argv[3] || path.join(__dirname, 'sqlite3')); | ||
const source = path.resolve(path.sep, process.argv[3] || path.join(path.dirname(fileURLToPath(import.meta.url)), 'sqlite3')); |
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.
This is bad for ESM / CJS interop. __pathname
should still be used.
@@ -45,10 +55,11 @@ function Database(filenameGiven, options) { | |||
// Load the native addon | |||
let addon; | |||
if (nativeBinding == null) { | |||
addon = DEFAULT_ADDON || (DEFAULT_ADDON = require('bindings')('better_sqlite3.node')); | |||
addon = DEFAULT_ADDON || (DEFAULT_ADDON = bindings('better_sqlite3.node')); |
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.
Previously, the binding
function was lazy-loaded but now itis always loaded. This will hurt cold start performance.
const requireFunc = typeof __non_webpack_require__ === 'function' ? __non_webpack_require__ : createRequire(import.meta.url); | ||
addon = requireFunc(nativeBinding); |
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.
createRequire
is bad for CJS / ESM interop.
Proposition of switching package to ESM as it is widely supported by JS environments
https://nodejs.org/api/esm.html
https://www.electronjs.org/docs/latest/tutorial/esm
Checked benchmarks with esm updates