kentcdodds / nps-utils

Utilities for http://npm.im/nps (npm-package-scripts)
https://doclets.io/kentcdodds/nps-utils/master
MIT License
101 stars 23 forks source link

added the function and tests #23

Closed mikecann closed 6 years ago

mikecann commented 6 years ago

As discussed in: https://github.com/kentcdodds/nps-utils/issues/22

For this PR, I support the happy-path if you are using the standard yarn workspaces or lerna structure:

module.exports = {
  scripts: {
    client: importPackage("client"),
    server: importPackage("server"),
    shared: importPackage("shared"),
  }
}

Also support you optionally providing the path:

module.exports = {
  scripts: {
    client: importPackage({path: "./mycustomfolder/my-nps-scripts.js" })
  }
}

It detects "descriptions" and handles them correctly.

Wasnt too sure how to structure the required test files (client, server).

Fixed most of the es-lint issues but one, no arrow functions?

Plus i'm not sure how to actually functionally test this other than using console.log, perhaps you can advise?

codecov[bot] commented 6 years ago

Codecov Report

Merging #23 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #23   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      3    +2     
  Lines          62     80   +18     
=====================================
+ Hits           62     80   +18
Impacted Files Coverage Δ
src/packages/client/package-scripts.js 100% <100%> (ø)
...meotherstructure/server/some-other-scripts-name.js 100% <100%> (ø)
src/index.js 100% <100%> (ø) :arrow_up:

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 0490c51...d1fe219. Read the comment docs.

codecov[bot] commented 6 years ago

Codecov Report

Merging #23 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #23   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          62     79   +17     
=====================================
+ Hits           62     79   +17
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) :arrow_up:

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 0490c51...7da29a5. Read the comment docs.

mikecann commented 6 years ago

Probably this should respect the options object of the imported script too

kentcdodds commented 6 years ago

Would you like to add yourself to the contributors table?

mikecann commented 6 years ago

@kentcdodds thank you so much for the review, really helpful. I have fixed everything that I could. The only thing is those helpful error messages had to go due to limitations in jest mocking, the native errors are okay.

I have updated the readme to add the new method, I hope is is okay. Im not sure how the docs get generated so couldnt check that they look okay.

I ran into issues automatically testing this on multiple platforms, a simple solution was to replace '\' with '/' which seems to work on windows just fine these days.

kentcdodds commented 6 years ago

This is great. Thank you so much for iterating and teaching me through this process!

mikecann commented 6 years ago

@kentcdodds the pleasure was all mine!

kentcdodds commented 6 years ago

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

mikecann commented 6 years ago

Thats awesome thanks Kent. Just one thing, do the docs auto-generate after some time?

kentcdodds commented 6 years ago

Well... It's supposed to, but it never really worked well. It looks like things have been updated on the esdoc site and it seems to be working mostly, but it doesn't have your new method. I'm not sure why not. Why don't we give it a day and see. If it doesn't update then maybe we should just put docs in the README...

mikecann commented 6 years ago

Sure, sounds like a plan, perhaps they have some cronjob to check for updates.