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

Friendly error messages for node < 4 #219

Merged
merged 2 commits into from Sep 10, 2018

Conversation

Projects
None yet
4 participants
@sudo-suhas
Copy link
Contributor

commented Aug 5, 2017

This sets up please-upgrade-node for bin executable to show friendly message to the user suggesting node upgrade instead of showing a stack trace:

λ node --version
v0.12.18

# BEFORE
λ node .\bin\depcheck
E:\Projects\repos\depcheck\node_modules\yargs\node_modules\camelcase\index.js:4
        let isLastCharLower = false;
        ^^^
SyntaxError: Unexpected strict mode reserved word
    at exports.runInThisContext (vm.js:73:16)
    at Module._compile (module.js:443:25)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (E:\Projects\repos\depcheck\node_modules\yargs\lib\command.js:3:19)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)

# AFTER
λ node .\bin\depcheck
depcheck requires at least version 4 of Node, please upgrade
@sudo-suhas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2017

Could you please help me figure out how I can fix the error thrown during npm run depcheck-json? I tried adding an ignore flag for please-upgrade-node but that didn't help either.

Edit: Could this be a bug? please-upgrade-node is being used in bin/depcheck but still reported as unused.

/* eslint-disable no-console, prefer-arrow-callback */
require('please-upgrade-node')(require('../package.json'));

/* eslint-disable no-console, prefer-arrow-callback, comma-dangle */

This comment has been minimized.

Copy link
@lijunle

lijunle Oct 10, 2017

Member

Why disable comma-dangle rule here?

This comment has been minimized.

Copy link
@sudo-suhas

sudo-suhas Oct 11, 2017

Author Contributor

Otherwise, have to change the function call:

 require('../dist/cli')(
   process.argv.slice(2),
@@ -10,5 +12,5 @@ require('../dist/cli')(
   console.error,
   function exit(code) {
     process.exitCode = code;
-  }
+  },
 );

But dangling comma in function is not supported in node < 8:

errors in node < 8
λ node --version
v4.2.0

λ node .\bin\depcheck
E:\Projects\repos\depcheck\bin\depcheck:16
);
^

SyntaxError: Unexpected token )
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:414:25)
    at Object.Module._extensions..js (module.js:442:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:311:12)
    at Function.Module.runMain (module.js:467:10)
    at startup (node.js:134:18)
    at node.js:961:3

λ nvm use 6.11.3
Now using node v6.11.3 (64-bit)

λ node --version
v6.11.3

λ node .\bin\depcheck
E:\Projects\repos\depcheck\bin\depcheck:16
);
^

SyntaxError: Unexpected token )
    at createScript (vm.js:56:10)
    at Object.runInThisContext (vm.js:97:10)
    at Module._compile (module.js:542:28)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:389:7)
    at startup (bootstrap_node.js:149:9)

Since this file isn't transpiled, we should remove the dangling comma and disable the rule.

@mnkhouri

This comment has been minimized.

Copy link
Member

commented Sep 8, 2018

Hi @sudo-suhas , I apologize for the very long delay on this PR -- thank you for the contribution, I'd love to get it merged! Might you be able to do an npm install on your branch and commit the updated package-lock.json? This project is now using a lockfile (it did not have one when you first opened the PR)

@LinusU

This comment has been minimized.

Copy link
Member

commented Sep 8, 2018

Neat package! 🎉

feat: Friendly err msg for node < 4
Setup please-upgrade-node for bin executable to show friendly
message for node upgrade instead of stack trace

@sudo-suhas sudo-suhas force-pushed the sudo-suhas:feat-please-upgrade-node branch from dbc4476 to da961cd Sep 9, 2018

@sudo-suhas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2018

Done with rebase.

@LinusU

LinusU approved these changes Sep 9, 2018

Copy link
Member

left a comment

Awesome 👏

@LinusU

This comment has been minimized.

Copy link
Member

commented Sep 9, 2018

Haha, the build is failing because bin/depcheck isn't being treated as a javascript file.

I think that the easiest thing is just to rename it to bin/depcheck.js? When npm installs the package the binary will still be called depcheck. @mnkhouri what do you think?

edit: that would also help with syntax highlighting in editors ☺️

@sudo-suhas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2018

Yes, I had mentioned that problem in #219 (comment)

@LinusU

This comment has been minimized.

Copy link
Member

commented Sep 9, 2018

Ah right, missed that.

It's not being picked up because depcheck doesn't know that it's a JavaScript file.

@mnkhouri

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

I think that the easiest thing is just to rename it to bin/depcheck.js? When npm installs the package the binary will still be called depcheck. @mnkhouri what do you think?

Great idea! I didn't know about that behavior (documented here). I just tested it locally, and renaming to include .js does fix the issue mentioned in #219 (comment)

@sudo-suhas in addition to renaming the file to bin/depcheck.js, you would also need to change the bin key in package.json to reflect the new name -- then I think we'll be good to go! Thanks for the continued contributions on this pr :)

@codecov

This comment has been minimized.

Copy link

commented Sep 10, 2018

Codecov Report

Merging #219 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #219   +/-   ##
=======================================
  Coverage   98.04%   98.04%           
=======================================
  Files          29       29           
  Lines         461      461           
=======================================
  Hits          452      452           
  Misses          9        9

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5fea70...d15f2e0. Read the comment docs.

@mnkhouri mnkhouri merged commit 8e32807 into depcheck:master Sep 10, 2018

4 checks passed

codecov/patch Coverage not affected when comparing a5fea70...d15f2e0
Details
codecov/project 98.04% remains the same compared to a5fea70
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sudo-suhas sudo-suhas deleted the sudo-suhas:feat-please-upgrade-node branch Sep 10, 2018

@mnkhouri

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Thank you @sudo-suhas! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.