Add support for shareable modules #69
Conversation
Enables the config of `scripts.modules` which specifies an array of modules to include in the search space of script executables. Implements testdouble#68
|
cc: @stefanpenner |
|
@searls Could you review this? I am open to any feedback / changes you come up with. |
lib/resolve-script/glob-patterns.js
Outdated
|
|
||
| if (options && Array.isArray(options.modules)) { | ||
| options.modules.forEach(function (moduleName) { | ||
| const modulePath = resolvePkg(moduleName + '/scripts') |
There was a problem hiding this comment.
I'll be the first to admit scripty has pretty piss-poor Windows support, but seeing a '/' reminds me I need to ask whether this new feature works under Windows, given it'll be looking for a scripts-win directory?
See our appveyor build and if there's some way to make sure this is tested to behave as predicted under windows (literally just resolving scripts-win when on win platform?)
There was a problem hiding this comment.
I don't have access to the appveyor build. I get "Project not found or access denied."
For the "/" I can use https://nodejs.org/api/path.html#path_path_sep if that is what you are referring to. I don't have a dev windows setup for testing so do you have any suggestions on what you use?
There was a problem hiding this comment.
I should be more clear:
-
Windows command support is implemented in a custom way in Scripty. If you're on Windows, it will first look in a mirrored directory hierarchy under
scripts-win/, notscripts/. That means for you to keep Windows support working consistently, this feature would need to resolve windows paths in the same way other scripts are loaded (via this bit of code here). -
Whoops, fixed the Appveyor permission error. You can see a build log here. What I think this PR really needs to be sure it works is a "safe" (i.e. integration) test that uses a new set of minimal nested module fixtures to verify that script-module loading works as you expect it to, and that also exercises scripts-win. I think if you look under the
safe/path you should find readily imitatible tests to start from
lib/resolve-script/glob-patterns.js
Outdated
| globArray.push(path.join(moduleLocation, '*')) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
This method was pretty tidy previously and now has a 2-nested branch condition shoehorned into it. Could we extract this to a well named private function? Maybe:
return [
stuff,
that,
was,
here
].concat(arrayOfNewNestedStuff(options.modules))
yarn.lock
Outdated
| @@ -0,0 +1,1906 @@ | |||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
There was a problem hiding this comment.
scripty uses npm and an npm package-lock.json. Could you please use npm & update its package-lock? I'm uncomfortable with the idea of having two (potentially divergent) sources of truth as to the library's nested dependencies.
There was a problem hiding this comment.
Oops 😄 (total just assumed yarn... I wonder if there is a wrapper that would just delegate to either yarn or npm based on the presence of a yarn.lock or package-lock 🤔). I will get rid of this and use package-lock.json.
- Restructuring load-option.js - Cleaning up glob-patterns.js - Remove yarn.lock and use package-lock.json
|
@searls Thanks for the review! I believe I have addressed all of your comments (expect the windows one which I left a clarifying comment). Let me know if my response / changes make sense or if you have any other comments / suggestions. |
lib/resolve-script/glob-patterns.js
Outdated
|
|
||
| if (options && Array.isArray(options.modules)) { | ||
| options.modules.forEach(function (moduleName) { | ||
| const modulePath = resolvePkg(moduleName + '/scripts') |
There was a problem hiding this comment.
I should be more clear:
-
Windows command support is implemented in a custom way in Scripty. If you're on Windows, it will first look in a mirrored directory hierarchy under
scripts-win/, notscripts/. That means for you to keep Windows support working consistently, this feature would need to resolve windows paths in the same way other scripts are loaded (via this bit of code here). -
Whoops, fixed the Appveyor permission error. You can see a build log here. What I think this PR really needs to be sure it works is a "safe" (i.e. integration) test that uses a new set of minimal nested module fixtures to verify that script-module loading works as you expect it to, and that also exercises scripts-win. I think if you look under the
safe/path you should find readily imitatible tests to start from
thoov
force-pushed
the
shareable-scripts
branch
from
e95dd7b to
b01719f
Compare
|
@searls I added a "safe" test that exercises the win32 flow (thanks for the clarification and pointing me to those examples). I believe this to be in a good spot now. Could you review (glob-patterns.js and script-dirs.js) as those were the places that I changed the most between your last review and now. |
|
Landed in v.1.8.0! 🎊 |
Enables the config of
scripty.moduleswhich specifies an array of modules to include in the search space of script executables.Implements #68