bjarneo / extract-domain

Extract domain name from an URL
https://www.npmjs.com/package/extract-domain
MIT License
31 stars 4 forks source link

mjs module is using require() #22

Closed nickluger closed 1 year ago

nickluger commented 2 years ago

I'm getting a require is not defined error. (Actually getting Error: You must install psl library, but the real error is just swallowed by the catch…).

This is probably because, this package defines an ESM entry point:

  "exports": {
    "require": "./dist/extract-domain.js",
   // here => 
    "default": "./dist/extract-domain.modern.mjs"
  },

But the file extract-domain.modern.mjs is using a dynamic require to get psl on demand here.

which ends up in the .mjs-file.

The thing is, require does not exist in Node ESM.

In other words: The file is stating I'm ESM, while using a CJS-only feature.

bjarneo commented 2 years ago

That is not good. Thanks for reporting. Adding hacktoberfest label to it maybe someone will pick it up. If not I will look into it, just not sure when I got some spare time to do so.

crobinson42 commented 1 year ago

IMO, this line inside the runtime should be at the top level so it throws at startup instead of in the runtime execution. It should also be converted to an import and the build step can make the appropriate cjs/mjs builds.

try {
            const psl = require('psl');
            return psl.get(url.slice(offsetStart, offsetPath));
        } catch (_e) {
            throw new Error(
                'You must install psl library (https://www.npmjs.com/package/psl) to use `tld` option'
            );
        }
bjarneo commented 1 year ago

Currently looking into fixing this @nickluger

Drawback is of course I have to make the code async if the PSL library is used, which makes this a breaking code change.

@crobinson42 I understand what you mean by putting it at the top level, but in that case I can't really load it on demand as the lib huge: https://bundlephobia.com/package/psl@1.9.0