-
Notifications
You must be signed in to change notification settings - Fork 4
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
CLI + Scoped Packages #1
Changes from 3 commits
af212bd
d668774
2982520
420452e
c6a7224
5e2d479
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
#!/usr/bin/env node | ||
|
||
var symlinked = require("./") | ||
|
||
var help = false | ||
var dashdash = false | ||
var error = null | ||
var flags = [] | ||
|
||
var args = process.argv.slice(2).filter(function(arg) { | ||
if (dashdash) | ||
return !!arg | ||
else if (arg === "--") | ||
dashdash = true | ||
else if (arg === "--paths" || arg === "-p") | ||
flags.push("paths") | ||
else if (arg === "--roots" || arg === "-r") | ||
flags.push("roots") | ||
else if (arg === "--links" || arg === "-l") | ||
flags.push("links") | ||
else if (arg.match(/^(-+|\/)(h(elp)?|\?)$/)) | ||
help = true | ||
else if (arg.match(/^-/)) | ||
error = "Unknown flag: " + arg | ||
else | ||
return !!arg | ||
}) | ||
|
||
if (help || args.length > 1 || flags.length > 1 || error) { | ||
// If they didn't ask for help, then this is not a "success" | ||
var log = help ? console.log : console.error | ||
log("Usage: symlinked [<path>]") | ||
log("") | ||
log(" Finds all linked package names of an npm package.") | ||
log("") | ||
if (error) { | ||
log(" " + error) | ||
log("") | ||
} | ||
if (flags.length > 1) { | ||
log(" Maximum of one flag may be passed at a time. Received: " + JSON.stringify(flags)) | ||
log("") | ||
} | ||
log("Options:") | ||
log("") | ||
log(" -h, --help Display this usage info") | ||
log(" -p, --paths Get linked package paths") | ||
log(" -r, --roots Get linked package roots") | ||
log(" -l, --links Get linked package links") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking it might make sense to explicitly require using
For easier documentation I think it makes sense for the CLI to mirror the JavaScript API as much as possible. Right now the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM. Thinking about it now, it seems like the interface should be switched to this. LMK your thoughts.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this interface seems slightly more expressive and flexible for future needs 💯 What that your reasoning too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ryanve yeah, flags didn't align well to the underlying API. Flags could be either global ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ryanve the message under that |
||
process.exit(help ? 0 : 1) | ||
} else | ||
go(args[0] || ".") | ||
|
||
function go (dir) { | ||
var exec = symlinked[flags[0] || "names"] | ||
var results = exec(dir) | ||
for (var i = 0; i < results.length; i++) { | ||
console.log(results[i]) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Linting runs via
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,14 +9,23 @@ function search(dir) { | |
var context = path.resolve(dir) | ||
if (!fs.existsSync(context)) return [] | ||
var contents = fs.readdirSync(context) | ||
return contents.map(function(name) { | ||
var scopedRoots = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seeing as the scope is part of the package Current result
Expected result
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ryanve good catch, the scope should definitely be in the output, I'll get that fixed. If you feel strong about the indicator I can add it but think it would cut some usability if you wanted to pipe this command to another command ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ryanve just realized what you meant by |
||
var results = contents.filter(function (name) { | ||
var isBin = name === ".bin" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the reason for the bin check? I'm not sure I understand this part. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since |
||
var isScoped = name.charAt(0) === "@" | ||
if (isScoped) scopedRoots.push(path.join(dir, name)) | ||
return !isScoped && !isBin | ||
}).map(function(name) { | ||
var relative = path.join(dir, name) | ||
var found = new Found | ||
found.name = name | ||
found.path = path.resolve(relative) | ||
if (is(relative)) found.link = read(relative) | ||
return found | ||
}) | ||
return scopedRoots.reduce(function (_, scopedRoot) { | ||
return _.concat(search(scopedRoot)) | ||
}, results) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought this was lodash at first so if reducing let's avoid There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the filter step above was added to remove processing on scoped directories. For these, it needs to instead look up all their sub-directories (packages) and run it for them. This produces a multi-dimensional array. Main point of the reduce is to flatten the arrays. Running
scopedRoots [ "@foo", "@bar" ] const scopedResults = scopedRoots.map(search); scopedResults [
[ "@foo/a", "@foo/b" ],
[ "@foo/c" ]
] This would then need an extra step to reduce it to: [
"@foo/a",
"@foo/b",
"@foo/c"
] So instead of all that, I used reduce to flatten as it searches each scoped directory. I'm sure there are other ways to rewrite it, but that's the gist of it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ryanve thinking about it more, I think the entire |
||
} | ||
|
||
function read(p) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
"version": "0.3.0", | ||
"description": "Node utility to list packages that are npm linked", | ||
"main": "index.js", | ||
"bin": "bin.js", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this flat structure. |
||
"scripts": { | ||
"demo": "node demo.js", | ||
"lint": "eslint . --ext .js", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yarn global
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of repeating the install instructions in both sections we can do something like this near the top. I think we can downplay the
yarn
instructions and focus on the npm ones. Local install applies to the CLI version if using via npm scripts ornpx
so I think we should favor local in the documentation. I can finetune the documentation afterwards if you want but would probably do something like in the comments below if you want to adjust.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Install
Local install as above is best practice if you are using in a shared codebase because then all developers will use the same version. CLI can be used locally via
npx
or via npm scripts.npm install
has a--global
flag you can add if you prefer global use. Yarn can be used viayarn add
andyarn global
respectively if you prefer yarn.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npx
exampleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good points, and good eye on the
yarn global
bit. Will drop these in and totally cool if you want to fine-tune.