Error when beautifier is installed without unibeautify peer dependency
stevenzeck opened this issue · 17 comments
Steps to reproduce:
- Globally install an @Unibeautify beautifier
- Ensure that unibeautify is not installed globally
- Run the cli, e.g.
unibeautify list -l
Result: It throws an error:
(node:7924) UnhandledPromiseRejectionWarning: NestedError: Could not require module '@unibeautify/beautifier-eslint'
at requireg (/Users/stevenzeck/Desktop/Development/unibeautify/core/unibeautify-cli/node_modules/requireg/lib/requireg.js:14:11)
at beautifierNames.map.beautifierName (/Users/stevenzeck/Desktop/Development/unibeautify/core/unibeautify-cli/dist/index.js:27:16)
at Array.map (<anonymous>)
at loadBeautifiers (/Users/stevenzeck/Desktop/Development/unibeautify/core/unibeautify-cli/dist/index.js:26:41)
at findInstalledBeautifiers.then (/Users/stevenzeck/Desktop/Development/unibeautify/core/unibeautify-cli/dist/index.js:34:16)
Caused By: Error: Cannot find module 'unibeautify'
at Function.Module._resolveFilename (internal/modules/cjs/loader.js:581:15)
at Function.Module._load (internal/modules/cjs/loader.js:507:25)
at Module.require (internal/modules/cjs/loader.js:637:17)
at require (internal/modules/cjs/helpers.js:20:18)
at Object.<anonymous> (/usr/local/lib/node_modules/@unibeautify/beautifier-eslint/dist/src/index.js:3:23)
at Module._compile (internal/modules/cjs/loader.js:689:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
at Module.load (internal/modules/cjs/loader.js:599:32)
at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
at Function.Module._load (internal/modules/cjs/loader.js:530:3)
(node:7924) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:7924) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
Oh I think I experienced this error, too.
Should probably show a clean error message, something like unibeautify is required as a peer dependency for globally installed beautifiers
@Glavin001 did you have a chance to look into this?
Upgrading to the just-released version 0.3.0 from NPM, I get the same error with list -l and also when trying to beautify code. But only when I have @unibeautify/beautifier-prettydiff installed. After uninstalling it via NPM, I can beautify C code fine using @unibeautify/beautifier-clang-format:
$ unibeautify list -l
Supported languages
1. C
2. C++
3. Java
4. Objective-C
$ echo '{}' | unibeautify --language C
Using config: -style=file
{}
The error I got above turned out to be due to a missing prettydiff2 package needed by @unibeautify/beautifier-prettydiff. After doing npm install -g prettydiff2 it works fine.
Anyway, the exception thrown in that case seems to be the same as when unibeautify is the missing module. Would it be possible to provide a user-friendly error message that covers all missing modules, or is there some use case where doing so would be bad policy? I suppose it would be fine in the CLI but not sure about Unibeautify core.
Note, you'll have to uninstall ESLint and @unibeautify/beautifier-eslint locally as well to test this if you are running it via node ./dist/cli.js list -l.
Seems like this isn't an issue when running the local build of @unibeautify/cli.
EDIT: You have to remove @unibeautify/beautifier-eslint and ESLint from your local clone of @unibeautify/cli. That way it will try to find them globally instead of locally, at which point it will fail.
@Glavin001 I have no idea how to fix this.
Looking at the logs:
(node:7924) UnhandledPromiseRejectionWarning: NestedError: Could not require module '@unibeautify/beautifier-eslint'
at requireg (/Users/stevenzeck/Desktop/Development/unibeautify/core/unibeautify-cli/node_modules/requireg/lib/requireg.js:14:11)
at beautifierNames.map.beautifierName (/Users/stevenzeck/Desktop/Development/unibeautify/core/unibeautify-cli/dist/index.js:27:16)
at Array.map (<anonymous>)
The applicable code would be: https://github.com/Unibeautify/unibeautify-cli/blob/27993143f0cc461c99bc18d4aca3ab7c49faeea7/src/utils.ts#L28
@szeck87 Consider something like:
export function loadBeautifiers(beautifierNames: string[]): Unibeautify {
const beautifiers = beautifierNames.map(beautifierName => {
try {
return requireg(beautifierName).default;
} catch (error) {
const scaryMessage = error.message;
const isMissingUnibeautify = scaryMessage.indexOf("Cannot find module 'unibeautify'") !== -1;
if (isMissingUnibeautify) {
// You can change the error to be a little friendlier
const friendlyMessage = "unibeautify is required as a peer dependency for globally installed beautifiers";
throw new Error(friendlyMessage);
} else {
// Otherwise, use the original error
throw error;
}
}
});
return unibeautify.loadBeautifiers(beautifiers);
}Although, looking at the original error message, I would just leave this as is. The original error message seemed clear to me.
Is it possible to encode dependencies in NPM such that the cli package depends on the unibeautify core package? Then NPM would make sure the core is installed. Likewise, prettydiff2 could be listed as an NPM dependency for @unibeautify/beautifier-prettydiff, etc.
@Glavin001 it's still an unhandled promise rejection somewhere, that's what I'm trying to get rid of.
@szeck87 oops I forgot. How about:
export function loadBeautifiers(beautifierNames: string[]): Unibeautify {
const beautifiers = beautifierNames.map(beautifierName => {
try {
return requireg(beautifierName).default;
} catch (error) {
console.error(error);
return null;
}
}).filter(Boolean);
return unibeautify.loadBeautifiers(beautifiers);
}Is it possible to encode dependencies in NPM such that the cli package depends on the unibeautify core package? Then NPM would make sure the core is installed. Likewise, prettydiff2 could be listed as an NPM dependency for @unibeautify/beautifier-prettydiff, etc.
We already do what npm recommends, use peerDependencies.
https://github.com/Unibeautify/beautifier-prettier/blob/master/package.json#L31-L34
https://github.com/Unibeautify/beautifier-prettydiff/blob/master/package.json#L33-L36
https://github.com/Unibeautify/beautifier-js-beautify/blob/master/package.json#L31-L34
https://github.com/Unibeautify/beautifier-eslint/blob/master/package.json#L31-L34
The problem is npm simply reports a warning message to the user, instead of forcefully installing when missing 😞 .
The problem is npm simply reports a warning message to the user, instead of forcefully installing when missing 😞 .
I see. That's too bad 🙁 In that case we'll probably have lots of novice users hitting this error so this issue is definitely on point.
(node:7924) UnhandledPromiseRejectionWarning: NestedError: Could not require module '@unibeautify/beautifier-eslint'
Caused By: Error: Cannot find module 'unibeautify'
To me, this error message is clear to run npm install unibeautify. Although I also have a good idea of what is going on, so I'm likely biased.
We can do a a simple pattern match on the error message throw for Cannot find module 'unibeautify' and then disable an improved error message like I posted in #32 (comment)
if (isMissingUnibeautify) {
// You can change the error to be a little friendlier
const friendlyMessage = "Please run:\n npm install --global unibeautify";
throw new Error(friendlyMessage);
}
Another thing is we should not be recommending --global anymore, unless this is explicitly what the user wants. The current best practice is to install locally for all projects, so npm install --save-dev unibeautify unibeautify-cli @unibeautify/beautifier-eslint would be the recommended command.
@szeck87 is this actually resolved or accidentally closed by smart message?
I would say it's resolved. The UnhandledPromiseRejectionWarning was the main thing.