Skip to content

Convert project to ESM#136

Merged
kelson42 merged 2 commits intomainfrom
common-js-require
Jun 24, 2024
Merged

Convert project to ESM#136
kelson42 merged 2 commits intomainfrom
common-js-require

Conversation

@audiodude
Copy link
Copy Markdown
Member

@audiodude audiodude commented Jun 24, 2024

EDIT: This PR now converts the entire project to ESM. I have tested npm install and it seems to be working for me on Linux. A quick scan of the build scripts doesn't reveal any imported calls that are made in only one branch, so I think it's safe.

Original PR message below.


Trying to import @openzim/libzim into mwoffliner.

It was building fine but I was getting this error when trying to run the JS:

$ node ./lib/cli.js --webp --mwUrl="https://bm.wikipedia.org/" --format="nopic:nopic" --format="novid:maxi" --osTmpDir="/dev/shm" --adminEmail="contact@kiwix.org" --outputDirectory="output" --customZimFavicon="https://drive.farm.openzim.org/wikipedia_all/favicon-48x48.png" --customZimDescription="offline version of Wikipedia in Bambara" --publisher="openZIM" --forceRender=WikimediaMobile
file:///home/tmoney/code/mwoffliner/lib/util/misc.js:11
import { StringItem } from '@openzim/libzim';
         ^^^^^^^^^^
SyntaxError: Named export 'StringItem' not found. The requested module '@openzim/libzim' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '@openzim/libzim';
const { StringItem } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:123:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:191:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:337:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:106:12)

Node.js v18.20.3

When I tried a test.ts with the following:

import * as libzim from '@openzim/libzim'

console.log(libzim)

I got:

 node lib/test.js
(node:1259347) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
/home/tmoney/code/node-libzim/dist/index.js:1
import bindings from "bindings";
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at internalCompileFunction (node:internal/vm:76:18)
    at wrapSafe (node:internal/modules/cjs/loader:1283:20)
    at Module._compile (node:internal/modules/cjs/loader:1328:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1422:10)
    at Module.load (node:internal/modules/cjs/loader:1203:32)
    at Module._load (node:internal/modules/cjs/loader:1019:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:203:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:337:24)
    at async loadESM (node:internal/process/esm_loader:34:7)

Node.js v18.20.3

Which led me to believe that upstream (this package) had the error.

As far as I understand, the import keyword can only be used in ESM and Typescript, so this does appear to be a bug.

@audiodude audiodude force-pushed the common-js-require branch from d458cb1 to 18930a0 Compare June 24, 2024 19:10
@audiodude audiodude changed the title Use CommonJS require instead of ESM import, since this package uses CommonJS Convert project to ESM Jun 24, 2024
@kelson42 kelson42 self-requested a review June 24, 2024 19:46
@kelson42 kelson42 merged commit 5193e76 into main Jun 24, 2024
@kelson42 kelson42 deleted the common-js-require branch June 24, 2024 19:46
@kelson42 kelson42 added this to the 3.2.1 milestone Jun 24, 2024
Comment thread src/index.js
@@ -1,4 +1,4 @@
import bindings from "bindings";
const bindings = require("bindings");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the wrong way around? I keep getting that error and fixed it by changing it back to import bindings from "bindings";

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, sorry! This PR was originally to fix the fact that the code was improperly using 'import' when it should have been using 'require'. Then we decided to move the whole thing to ESM and I forgot to revert that file.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries! Glad it works now. Thanks for maintaining this repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants