Skip to content
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

fix crash when using from cli #65

Closed
wants to merge 1 commit into from
Closed

fix crash when using from cli #65

wants to merge 1 commit into from

Conversation

dgobaud
Copy link
Contributor

@dgobaud dgobaud commented Aug 8, 2019

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

require.main is undefined when running from cli - don't use unless it exists
@Kehrlann
Copy link
Collaborator

Kehrlann commented Sep 5, 2019

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
Copy link
Contributor Author

dgobaud commented Sep 5, 2019

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
Copy link
Collaborator

Kehrlann commented Sep 5, 2019

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

@dgobaud
Copy link
Contributor Author

dgobaud commented Sep 5, 2019

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
Copy link
Collaborator

Kehrlann commented Sep 5, 2019

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 🙇‍♂️

@dgobaud
Copy link
Contributor Author

dgobaud commented Sep 5, 2019

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

@Kehrlann
Copy link
Collaborator

Kehrlann commented Sep 8, 2019

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
Copy link
Contributor Author

dgobaud commented Sep 8, 2019

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
Copy link
Collaborator

Kehrlann commented Sep 13, 2019

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(""));
  1. 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.

  2. 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
Copy link
Contributor Author

dgobaud commented Sep 13, 2019

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
Copy link
Collaborator

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 ?

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.

2 participants