-
Notifications
You must be signed in to change notification settings - Fork 333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow glob pattern as arg for packageFile cli option. closes #522 #785
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
It looks good. A few additional considerations:
-
Testing - We should have at least once test for this new behavior. I would suggest something like this in
bin.test.js
:it('--deep', async () => { const output = await spawn('node', [path.join(__dirname, '../bin/cli.js'), '--jsonUpgraded', '--deep'], { cwd: path.join(__dirname, 'ncu/deep') }) ... })
-
Performance - It's expensive to run
analyzeProjectDependencies
multiple times as-is since it will duplicatenpm view
for dependencies that show up in multiple package files. It would be more efficient to determine all of the versions up front and pass that toanalyzeProjectDependencies
to bypass the typical lookups. This of course adds additional work. In lieu of this, I think a warning message and invitation to contribute would be adequate. I think users would expect dependencies to get de-duped, so a warning here is appropriate. What do you think? -
Breaking - I believe this is a breaking change since
packageFile
is now interpreted as a glob pattern, and it's possible that a value that someone used before will not be interpreted the same way. No big deal, just wanted to make that explicit here.
Added some tests...
Not sure what would be best option here... should
It's should not have breaking changes or what somebody could have passed to npm-check-updates/lib/index.js Lines 451 to 457 in a53e922
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look really great, thank you for taking the time! Let's rename monorepo
to deep
to create a closer association between the tests and what feature is being tested.
Yes, we would probably want to cache the results from queryVersions. Given the additional work, I'm okay with a warning message instead. If we do take it on, we should merge this first and do that work in a separate PR, since this PR has a good scope right now.
|
updates also make use of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updates also make use of
--prefix
flag e.g. to check everything in~/projects
directory regardless of initialpwd
ncu --deep --prefix '~/projects
Great idea! However the --prefix
option is for overriding the npm prefix
. --cwd
is used to change where the current working directory.
Ok that was a BUG I created before introducing the feature 😞 . It's fixed now
|
ce8e85c
to
fdd04c3
Compare
…hine#522 Implement --deep flag as alias of `--packageFile '{,*[!node_modules]/**/}package.json'`
Allow analyzeProjectDependencies to collect json output and return analysis data
Since |
Thanks for being responsive to feedback and helping to expand the functionality of |
@mkungla This was WAY easier than I thought it would be! 0f81e6d |
@raineorshine that's nice and to follow up
npm-check-updates/lib/index.js Lines 425 to 428 in c1e2f3d
|
Yes, I forgot to run
Great, done. |
Allow
glob
as arg for--packageFile
and implement --deep flag as alias of--packageFile '{,*[!node_modules]/**/}package.json'
closes #522