ilearnio / module-alias

Register aliases of directories and custom module paths in Node
MIT License
1.76k stars 69 forks source link

fix crash when using from cli #65

Closed dgobaud closed 5 years ago

dgobaud commented 5 years ago

require.main is undefined when running from cli - don't use unless it exists

Kehrlann commented 5 years ago

Hi @dgobaud, thanks for contributing 👋

Could you please add a bit of context to this PR ? Specifically, I'm looking for repro steps for the bug this PR will fix. I'm not sure I understand what the bug is.

Also, before accepting a PR we usually want a few test cases specifying the behavior.

Cheers !

dgobaud commented 5 years ago

yes try this

node -i
> moduleAlias = require("module-alias");
{ [Function: init]
  addPath: [Function: addPath],
  addAlias: [Function: addAlias],
  addAliases: [Function: addAliases],
  isPathMatchesAlias: [Function: isPathMatchesAlias],
  reset: [Function: reset] }
> 
> 
> moduleAlias.addPath(path.resolve(""));
TypeError: Cannot read property 'paths' of undefined
    at Function.addPath (/.../node_modules/module-alias/index.js:85:38)
Kehrlann commented 5 years ago

I see. What would the use-case be for trying to run module-alias from the cli ?

dgobaud commented 5 years ago

I have a Firebase typescript project that uses it. Typescript compiles it to JS in lib folder. I have a utils folder. To make it work when running the compiled JS I do

moduleAlias = require("module-alias");

// because of https://github.com/ilearnio/module-alias/pull/65
try { moduleAlias.addPath(path.resolve("")); } catch (e) {};

utils = require("./utils");
Kehrlann commented 5 years ago

Mmmh I tried a basic example myself and it worked. I'd like to better understand the problem before fixing it with those if-checks.

I probably missed a few details. Could you maybe share a sample codebase showcasing the problem ? In the form of a small github repo ? Anyway, thanks a lot for contributing and helping use improve module-alias :bowing_man:

dgobaud commented 5 years ago

it works BUT you can see the exception above itll crash your code if you dont catch it

Kehrlann commented 5 years ago

I can't reproduce it locally, see this repo: https://github.com/Kehrlann/module-alias-typescript . Feel free to send you own minimal repo so that I can take a look at it.

dgobaud commented 5 years ago

but you don't need type script just do the following

node -i moduleAlias = require("module-alias"); moduleAlias.addPath(path.resolve(""));

and you get below error

TypeError: Cannot read property 'paths' of undefined at Function.addPath (/.../node_modules/module-alias/index.js:85:38)

Kehrlann commented 5 years ago

Ok I finally got some time to look into it. Summary of the findings:

  1. As stated previously, when running from the node REPL (e.g. just running node), the following code throws an exception, because require.main is undefined:

    const moduleAlias = require("module-alias");
    moduleAlias.addPath(path.resolve(""));
  2. However, the code has a side-effect: it adds the resolved path to module-alias' internal modulePaths array. If we guarded against require.main being undefined, we would be able to add to modulePaths without the exception. Having this allows to resolve some packages using require.resolve.

  3. This has a single use case. Imagine you have a utils package at the root of your directory. You then run a REPL and execute:

    
    require('./utils'); // <-this always works ✅
    require.resolve('utils', { paths: [""] }); // <- this always succeeds ✅
    require.resolve('utils', { paths: ["/some/path"] }); // <- this fails before using addPath ❓❌

const path = require('path'); const moduleAlias = require("module-alias"); moduleAlias.addPath(path.resolve("")); // whether this throws or not does no affect the rest of the script

require('utils'); // <- this fails ❌ require.resolve('utils'); // <- still fails ❌ require.resolve('utils', { }); // <- still fails ❌ require.resolve('utils', { paths: ["/some/path"] }); // <- this now works ❓✅



Then the question is, is it worth it to guard against `require.main` being undefined for a user that:
- wants to use `require.resolve('some-module', { paths: ["some/unrelated/path"] })` after `moduleAlias.addPaths("my/modules")` in the REPL
- when instead they could just use `require.resolve('some-module', { paths: ["some/unrelated/path", "my/modules"] })` without module-alias ?

I don't think it's worth it, unless there are other more compelling use-cases.
dgobaud commented 5 years ago

Ok so i found the issue it is more complicated. From the REPL if I require a file that itself does require ('consts') if i don't have below 2 module-alias lines then it fails.

Below is example generated JS from my TS project that uses module-alias. When I import it in REPL I must first have below module-alias setup or it will fail.

moduleAlias = require("module-alias");

// because of https://github.com/ilearnio/module-alias/pull/65
try { moduleAlias.addPath(path.resolve("")); } catch (e) {};
"use strict";
var __importStar = (this && this.__importStar) || function (mod) {
    if (mod && mod.__esModule) return mod;
    var result = {};
    if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) result[k] = mod[k];
    result["default"] = mod;
    return result;
};
Object.defineProperty(exports, "__esModule", { value: true });
const js_lib_1 = require("@passfolio/js-lib");
const consts = __importStar(require("consts"));
exports.emailAddComplianceAddresses = (email) => {
    if (['production', 'beta'].includes(js_lib_1.kms.settings.CONFIG_ENV)) {
        email.bcc = consts.email.COMPLIANCE_EMAIL;
    }
};
//# sourceMappingURL=emailAddComplianceAddresses.js.map
Error: Cannot find module 'consts'
    at Function.Module._resolveFilename (module.js:548:15)
    at Function.Module._load (module.js:475:25)
    at Module.require (module.js:597:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/path/functions/lib/utils/emailAddComplianceAddresses.js:11:29)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Module.require (module.js:597:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/path/functions/lib/utils/index.js:13:10)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
Kehrlann commented 5 years ago

Ok I see. Sounds good to me ! Thanks for taking the time to explain.

Could you please add test cases so we can merge this ?