added a working version of my example

[jkroso]

and fixed some stuff

[DamonOehlman]

@jkroso Definitely keen to pull these changes in, but would like to make sure that @ForbesLindesay is happy with the changes too as it tweaks his code also.

Nice work on the run script too btw - very useful and saves having all the deps at the top level, which was something I wasn't happy with.

[jkroso]

AFIK it wouldn't happen in older nodes because path.join was less fussy then. Still a funny bug with .bind though

[Raynos]

You forgot .map(path.basename); everywhere.

[jkroso]

@raynos they all return [.git, child-dirs, node_modules] which is correct isn't it?

[ForbesLindesay]

Yes, that's correct. @Raynos you only need to .map(path.basename) if you've previously done .map(path.join.bind(null, root).

I'm fine with the changes to my code, they all look good. I do have a couple of concerns though:

1. Shouldn't we keep all the dependencies in a single package.json in the root directory so you only have to run npm install once to get all dependencies of all implementations.
2. We should write a node.js script to run the implementations using fs.readdir and cp.exec (or win-spawn npm module or just require them in because cp.spawn has compatibility issues). The current shell script wont run natively on windows.

[jkroso]

Come to think of it my dir + '/' + name code probably doesn't work in windows either. I'm happy if you wan't to replace the shell script with a node script though I'd like to keep the package.json's separate for the sake of not making a mess. People (me) might like to use post install hooks (if they worked). lol everything is so broken

[ForbesLindesay]

Actually, as it goes, dir + '/' + name, works fine. All the node.js methods accept either / and \ and treat them equivalently. Windows doesn't allow either within a file or folder name so there's no reason not to.

postinstall works doesn't it? as does preinstall which would probably be sufficient in this case if we used cp.exec?

[jkroso]

I was going to use

{"scripts":{"install":"npm depupe"}}

but dedupe was too broken. So its dedupe not the post install hooks that are broken sorry. hence my "result-type" dep which doesn't look like its being used. I just think its better to put an exec('npm i') in the node script than to put everything in the top level json file. safer.

[DamonOehlman]

Will merge this in now, any tweaks that are required can be applied after that. Thanks guys for your efforts :)

[ForbesLindesay]

@jkroso

Fine with me, it's certainly cleaner. I'd also be in favor of each demo getting a (brief) readme to describe what's going on. Most of them are not that obvious.

[jkroso]

swt i'll write up the run.js script now