msurdi / frontend-dependencies

Copies node packages to a directory where your frontend tools will be able to find them
24 stars 5 forks source link

Make this package CLI compatible #12

Closed evanplaice closed 5 years ago

evanplaice commented 5 years ago

It wouldn't take much work to extend this so it can work as a standalone CLI package.

It would need a CLI entry-point like https://github.com/evanplaice/node-ftpsync/blob/master/bin/ftpsync. Then set the bin property to point at the entry point.

Once that's done it should be able to run the script directly from the CLI by.

Installing locally then:

npm run [package-name]

Installing globally then in the CLI:

[package-name]

Launching via NPX without installing:

npx [package-name]

This could also be extended to run an alternate config.json by specifying a --config argument from the cli.

FelixFurtmayr commented 5 years ago

Hi evanplaice, I guess you may have a problem to solve, but right now I don't the usecase. Can you tell your usecase? Don't get me wrong - I'm not against your idea; so far I just did not get the point of "why do it" and where the benefit is.

Regards Felix

evanplaice commented 5 years ago

Makes it easier for users to interact with the tool. Treats it more like a fully-featured package manager rather than just a post-install script.

FelixFurtmayr commented 5 years ago

So you mean you would use the package via separate script, to copy files from npm packages to desired destinations. Not just on "npm install". That makes sense for me and sounds like a good idea. And it is not in contrast to anything the package does right now.

@msurdi what do you think?

@evanplaice could you do a pull request? I'm also willing to help with the implementation next week if msurdi is ok with that.

msurdi commented 5 years ago

It makes sense to me. I haven't had this need but I understand this could be useful in case you want to perform the copying operations manually, instead of having to rely on it running automatically after an npm install command. So 👍 .

FelixFurtmayr commented 5 years ago

@evanplaice So we like your idea. Can you do a pull request? Do you need some help with it?

I had a look at the code and I think that we need those changes:

  1. more options to pass the additional infos
  2. info in the readme about this options
  3. CLI entry point file
  4. update testcases

Notes to "more options"

// I would change this line:
function frontendDependencies(workDir) { 

// to
function frontendDependencies(options) { 
// advatage: we can have plenty options in the future and won't have to break old structures
// disadvatage: old projects that may have used the "workDir" option will break. So we need a major version update. For me this would be fine. 

    // prepare everything
    options = options || {};
    workDir = options.workDir || process.cwd();
    packageJsonPath =  options.packageJsonPath || workDir;
    var packageJson = getAndValidatePackageJson(packageJsonPath); // this function has to be modified
evanplaice commented 5 years ago

So you want built-in defaults. Cool. Does this need to maintain backward compatiblity with ES5?

I'm currently working on another project, I'll start work on the PR once I finish that.

FelixFurtmayr commented 5 years ago

No, we need no backwards Compatibility with ES5. I had this issue the last days. All old node version (version 4 or below) have run out of support or will in the near future.

What will be a breaking change is rather if we pass "options" instead of the "workDir". A pitty that I was not smart enough to build it like that from the beginning. But that change seems ok for me - as long as we clearly tell in the readme and have major version update.

So you might just do the pull request when you are ready and get the feedback from msurdi and me.

FelixFurtmayr commented 5 years ago

ok ve3, was just to lazy to do another pull request myselfe since I had the repo not cloned.

I put your improvement of the docs in my pull request, fixed the test, fixed the original issue by always adding quotes.

Your contribution to this package is appreciated. Thank you. We would be happy to see another pull request from you in the future.

By the way: the error you got running "npm test" was because you had "mocha" not installed. You need to do that before.

FelixFurtmayr commented 5 years ago

when msurdi merges the request, everything should work - put any further comments on the topic in the pull request. closing the issue.