CLI + Scoped Packages #1

cchamberlain · 2017-11-16T21:42:47Z

CLI

I based the structure off of https://github.com/isaacs/rimraf (written by npm's inventor Isaac Schlueter).

Installing globally and running symlinked at CLI in a package directory prints all package names, one per line (the names function). You can also test is out by pulling, running npm link from the repo root, then running symlinked in any directory should work.

I added support for the paths, roots, and links options. Passing more than one of these flags or an unknown flag will cause it to print help text with a useful error message.

Scoped Packages

During my testing I found that it wasn't picking up on symlinks of scoped packages (@organization). This is a very widely needed use case for symlinked packages when working within an organization so I added support for it.

ryanve

This is awesome! Thanks for all this! I think we can make some improvements and then get it merged. See review comments for details. If scoped support and CLI are independent features then they could be separate PRs as to facilitate easier review and faster merge because one would not block other. But see the comments and let me know what you think 🤓

ryanve · 2017-11-18T20:49:21Z

package.json

@@ -3,6 +3,7 @@
  "version": "0.3.0",
  "description": "Node utility to list packages that are npm linked",
  "main": "index.js",
+  "bin": "bin.js",


I like this flat structure. rimraf was a great example to follow 👍

ryanve · 2017-11-18T21:12:56Z

bin.js

+  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")


I'm thinking it might make sense to explicitly require using --names or at least support the flag. Now --names appears unknown because -n, --names Get linked package names is not provided.

$ node bin --names Usage: symlinked [<path>] Finds all linked package names of an npm package. Unknown flag: --names Options: -h, --help Display this usage info -p, --paths Get linked package paths -r, --roots Get linked package roots -l, --links Get linked package links

For easier documentation I think it makes sense for the CLI to mirror the JavaScript API as much as possible. Right now the symlinked export is an object but if a future release made it be a function then we might want to reserve the argless case for that. Requiring at least one flag seems safer at least as a first go. What do you think?

SGTM. Thinking about it now, it seems like the interface should be switched to this. LMK your thoughts.

Usage: symlinked <command> [<path>] Finds all linked package names of an npm package. Commands: names Get linked package names paths Get linked package paths roots Get linked package roots links Get linked package links Options: -h, --help Display this usage info

Yes this interface seems slightly more expressive and flexible for future needs 💯

What that your reasoning too?

@ryanve yeah, flags didn't align well to the underlying API. Flags could be either global (--help) or vary by command - the model I had would not have worked well with the latter. The bin.js code is simpler now.

@ryanve the message under that Usage text doesn't really fit anymore. Might want to just edit inline before merge.

ryanve · 2017-11-18T21:28:50Z

index.js

@@ -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 = []


Seeing as the scope is part of the package name then I think it makes sense for the names result to include the scope. Right? Would additionally adding scope info like found.scope = "@example" be useful as an indicator?

Current result

$ npm link eol $ npm link @songkick/promise-retry $ node bin eol promise-retry

Expected result

$ npm link eol $ npm link @songkick/promise-retry $ node bin eol @songkick/promise-retry

@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 (rimraf for example). Think it would make for nice content to a --pretty or --verbose flag at some point in future. Conversely, a flag could be passed down the line to specify only raw output. Whichever road you prefer is cool.

@ryanve just realized what you meant by found.scope =, going to add that. Can ignore the comment above.

ryanve · 2017-11-18T21:33:05Z

bin.js

+  var results = exec(dir)
+  for (var i = 0; i < results.length; i++) {
+      console.log(results[i])
+  }


Linting runs via pretest you can do npm install and then npm test to check linting

58:7 error Expected indentation of 4 spaces but found 6 indent

ryanve · 2017-11-18T21:58:02Z

README.md

+
+```
+yarn add global symlinked
+```


yarn global

Unlike the --global flag in npm, global is a command which must immediately follow yarn. Entering yarn add global package-name will add the packages named global and package-name locally instead of adding package-name globally.

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 or npx 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.

Install

npm install symlinked

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 via yarn add and yarn global respectively if you prefer yarn.

npx example

npx symlinked --paths

All good points, and good eye on the yarn global bit. Will drop these in and totally cool if you want to fine-tune.

ryanve · 2017-11-18T22:56:24Z

index.js

-  return contents.map(function(name) {
+  var scopedRoots = []
+  var results = contents.filter(function (name) {
+    var isBin = name === ".bin"


What is the reason for the bin check? I'm not sure I understand this part.

Since .bin is a special folder (never an npm package itself), this is filtering it from the results to be processed in the following map step. Not filtering it adds extra work and could lead to other issues since the map step is expecting these folders to be packages (AFAIK).

ryanve · 2017-11-18T23:03:29Z

index.js

    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)


I thought this was lodash at first so if reducing let's avoid _ as the accumulation variable name. I actually wonder if there is another technique we can use above that would avoid the need to reduce. Is the scoped solution to just search deep or is there more to it than that?

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 search on each of the scopedRoots directories produces a new array of results.

@foo/ a/ b/ @bar c/ d/ e/

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.

@ryanve thinking about it more, I think the entire filter / map / reduce would be better as a single reduce. I'll kick something up in next commit.

cchamberlain · 2017-11-19T00:30:37Z

@ryanve replied to comments, going to follow up with stuff that's firm. Sorry for the large size, my use case is with scoped packages, which caused me to go down this tangent. In general, I'm a fan of small PRs and simplicity. 😸

cchamberlain · 2017-11-19T02:39:06Z

@ryanve think I hit everything. Feel free to change anything as you see fit or kick it back if you have requests. Great review suggestions 👍 .

ryanve

CLI is 💯 Thanks for updating! Just minor notes on the scope support and then this seems good to merge =)

ryanve · 2017-11-19T03:29:15Z

bin.js

+    return !!arg
+})
+
+var argsOk = args.length >= 1 && args.length <= 2 && Object.keys(symlinked).indexOf(args[0]) !== -1


argsOk logic can simplify to this unless there's a reason otherwise

args.length === 1 && symlinked.hasOwnProperty(args[0])

@ryanve - main reason for the 1-2 args was to allow an optional directory to be passed as last argument. Cool with me if you want to drop that but its using args[1] || "." on line 44. I like the hasOwnProperty simplification - will kick that up in a second.

ryanve · 2017-11-19T03:33:22Z

index.js

-  })
+    accumulated.push(found)
+    return accumulated
+  }, [])


Cleaner reducing ✅

I generally hesitate about adding extra arguments so I'd be open to an alternative that handles scope privately but it seems okay as is for now seeing as the search method is unlisted and that we are in 0.x version. What we can do is merge and then do a release to try it out and see. Sound good? The reason I hesitate is about keeping the exposed interface simple and consistent across methods.

@ryanve sounds good. I think it would probably be a bit better if the scope argument was reworked to be used in a filtering context. I'm out of time this week, leaving on vacation tomorrow so think its probably best to release then worry about it in the future (given its private).

Filtering in more ways could be useful. Interesting idea. Enjoy vacation =)

ryanve · 2017-11-19T04:00:58Z

index.js

  dir = dir || "."
  var context = path.resolve(dir)
  if (!fs.existsSync(context)) return []
  var contents = fs.readdirSync(context)
-  return contents.map(function(name) {
+  return contents.reduce(function (accumulated, name) {
+    if (name === ".bin") return accumulated; // .bin directory is never a package


.bin would get filtered out anyway by is right? Just wondering is there another reason to include?

@ryanve I don't feel very strong about this one so I'm dropping it for now. Believe that is step is running both fs.existsSync and fs.lstatSync which is touching the file system twice unnecessarily and in the weird instance where .bin is a link, I think the results would be pretty wacky since it may interpret it as a package (thinking of alternate npm clients that may do things in unexpected ways).

On side note, I think is would be better off as:

function is(p) { if (p instanceof Found) return p.hasOwnProperty("link") try { return fs.lstatSync(p).isSymbolicLink() } catch (err) { return false } }

Reasons are that this makes the operation atomic, faster (less reads), and the majority cases where it wouldn't exist are exceptional. Node's primary API deprecated fs.exists long ago for these reasons - https://nodejs.org/api/fs.html#fs_fs_exists_path_callback

Also, seems like is might be better off as two separate functions isLink and hasLink, given that its doing completely different things depending on the argument - having it all in one made debugging more difficult for me as a noob. $.02 😄

Good info. I can do a follow-up to address that. I did the is function like that to be terse but has could be separate. This was the reason for doing var found = new Found instead of using a plain object var found = {}

ryanve · 2017-11-19T04:25:37Z

package-lock.json

+      "dev": true
+    }
+  }
+}


cchamberlain · 2017-11-21T20:21:03Z

@ryanve just pushed update to remove .bin special case check and the better hasOwnProperty check on the command. Feel free to edit anything or kick back if there are additional requests. I'll be out of town Wednesday night through next Monday so anything big will incur some delays - minor stuff is nbd.

cchamberlain · 2017-11-21T20:35:52Z

One more note on #1 (comment), it appears that existsSync internally wraps a stat call https://github.com/nodejs/node/blob/master/lib/fs.js#L342

Heated discussion on the topic: nodejs/node#1592 (comment)

ryanve · 2017-11-21T20:54:29Z

Awesome! I'll merge and make a release that we can try. Enjoy vacation and thanks again for all the comprehensive work =)

* feat: cli and scoped packages support with updated docs * removed extraneous function * fixed quote style * simplified search, adjusted cli to use commands, and updated docs * updated readme * refactor: simplified command exists check and .bin special case removal

ryanve · 2017-11-21T21:47:53Z

🎉 Released in 0.5.0 and published to npm so you can do any of these

npm install symlinked@0.5.0
npm install symlinked@latest
npm install symlinked

Cole Chamberlain added 3 commits November 16, 2017 13:28

feat: cli and scoped packages support with updated docs

af212bd

removed extraneous function

d668774

fixed quote style

2982520

ryanve approved these changes Nov 18, 2017

View reviewed changes

Cole Chamberlain added 2 commits November 18, 2017 18:29

simplified search, adjusted cli to use commands, and updated docs

420452e

updated readme

c6a7224

ryanve approved these changes Nov 19, 2017

View reviewed changes

refactor: simplified command exists check and .bin special case removal

5e2d479

ryanve merged commit fcc782d into ryanve:master Nov 21, 2017

ryanve mentioned this pull request Nov 21, 2017

Retroactively bump 0.4.0 #2

Merged

CLI + Scoped Packages #1

CLI + Scoped Packages #1

Conversation

cchamberlain commented Nov 16, 2017

CLI

Scoped Packages

ryanve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchamberlain Nov 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Current result

Expected result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanve Nov 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Install

Choose a reason for hiding this comment

npx example

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchamberlain Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchamberlain commented Nov 19, 2017

cchamberlain commented Nov 19, 2017

ryanve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchamberlain commented Nov 21, 2017

cchamberlain commented Nov 21, 2017

ryanve commented Nov 21, 2017

ryanve commented Nov 21, 2017

cchamberlain Nov 18, 2017 •

edited

Loading

ryanve Nov 18, 2017 •

edited

Loading

`npx` example

cchamberlain Nov 19, 2017 •

edited

Loading