Lookup crashes when module is not in project root.

[phyllisstein]

Just flagging that this turns out to be a little too direct a lookup technique:

atom-autocomplete-modules/src/utils/export-module-completion.js

Lines 51 to 52 in 2961f55

	const nodeModulePath = path.normalize(`${projectPath}/node_modules/${importModule}`);
	const mainfile = getMainFileName(nodeModulePath);

It was crashing for me in a monorepo, where the folder structure looks something like this:

.
├── node_modules
├── packages
│   ├── astriflammante
│   │   ├── node_modules
│   │   └── package.json
│   ├── crane
│   │   ├── node_modules
│   │   └── package.json
│   ├── empire
│   │   ├── node_modules
│   │   └── package.json
│   ├── marilyn
│   │   ├── node_modules
│   │   └── package.json
│   ├── sarastro
│   │   ├── node_modules
│   │   └── package.json
│   └── wendy
│       ├── node_modules
│       └── package.json
└── package.json

If I was working in wendy and importing something from react, the package would crash trying to read a nonexistent ./node_modules/react/package.json instead of ./packages/wendy/node_modules/react/package.json:

Uncaught (in promise) Error: ENOENT: no such file or directory, open '/Users/daniel/Repos/Bauer/oedipus/node_modules/react/package.json'
    at Object.fs.openSync (fs.js:558:18)
    at Object.module.(anonymous function) [as openSync] (ELECTRON_ASAR.js:173:20)
    at Object.fs.readFileSync (fs.js:468:33)
    at Object.fs.readFileSync (ELECTRON_ASAR.js:506:29)
    at getMainFileName (/Users/daniel/Repos/Personal/Forks/atom-autocomplete-modules/src/utils/export-module-completion.js:58:22)
    at parseModule.then.results (/Users/daniel/Repos/Personal/Forks/atom-autocomplete-modules/src/utils/export-module-completion.js:52:24)

Kind of an edge case, but might be worth at least wrapping in a try...catch block to keep it from crashing other autocompletion modules.

[jonyeezs]

Don't think this was made for monorepo in mind. I reckon the codebase could use a bit of refactoring to make it easier to implement this. Do you have a solution in mind?

[phyllisstein]

You're right, most of its functionality doesn't work in the kind of environment I'm describing. But throwing this error rather than gracefully handling it happens to crash the autocomplete module badly enough that no other packages' completions are returned either.

Given how bad the failure case is, and given that making this project work more broadly for monorepos isn't on the table right now, I think just catching the error would be sufficient.

[jonyeezs]

Happy for you to put in a PR 👍

…

On Thu., 21 Dec. 2017, 8:42 pm Daniel P. Shannon, ***@***.***> wrote: You're right, most of its functionality doesn't work in the kind of environment I'm describing. But throwing this error rather than gracefully handling it happens to crash the autocomplete module badly enough that no other packages' completions are returned either. Given how bad the failure case is, and given that making this project work more broadly for monorepos isn't on the table right now, I think just gracefully handling the error would be sufficient. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#89 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKXAhsB7MD4CCrizzNkzInLB4l_kGTm7ks5tClI2gaJpZM4RH5He> .