Skip to content
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

Merged
merged 22 commits into from
Jan 20, 2021

Conversation

mkungla
Copy link
Contributor

@mkungla mkungla commented Jan 12, 2021

Allow glob as arg for --packageFile and implement --deep flag as alias of
--packageFile '{,*[!node_modules]/**/}package.json'

closes #522

package.json Outdated Show resolved Hide resolved
Copy link
Owner

@raineorshine raineorshine left a 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 duplicate npm 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 to analyzeProjectDependencies 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.

package.json Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
@mkungla
Copy link
Contributor Author

mkungla commented Jan 14, 2021

  • 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') })
      ...
    })

Added some tests...

  • Performance - It's expensive to run analyzeProjectDependencies multiple times as-is since it will duplicate npm 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 to analyzeProjectDependencies 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?

Not sure what would be best option here... should versionmanager have some caching

  • 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.

It's should not have breaking changes or what somebody could have passed to --packageFile which would not fall to

else {
if (pkgs.length === 1 && pkgs[0] !== defaultPackageFilename) {
options.packageFile = pkgs[0]
}
const [pkgData, pkgFile] = await findPackage(options)
analysis = await analyzeProjectDependencies(options, pkgData, pkgFile)
}

Copy link
Owner

@raineorshine raineorshine left a 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.

lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
@raineorshine
Copy link
Owner

Not sure what would be best option here... should versionmanager have some caching

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.

It's should not have breaking changes or what somebody could have passed to --packageFile which would not fall to

npm-check-updates is particularly conservative since it is used in many CI setups. I can't definitively conclude that no user was utilizing a --packageFile that would now be interpreted as a glob and relying on unspecified behavior. Making this a breaking change allows developers to evaluate for themselves if there is any risk in upgrading.

mkungla added a commit to mkungla/npm-check-updates that referenced this pull request Jan 19, 2021
@mkungla mkungla requested a review from raineorshine January 19, 2021 22:36
@mkungla
Copy link
Contributor Author

mkungla commented Jan 19, 2021

updates also make use of --prefix flag e.g. to check everything in ~/projects directory regardless of initial pwd

ncu --deep --prefix '~/projects

Copy link
Owner

@raineorshine raineorshine left a 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 initial pwd

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.

@mkungla
Copy link
Contributor Author

mkungla commented Jan 20, 2021

updates also make use of --prefix flag e.g. to check everything in ~/projects directory regardless of initial pwd
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

ncu --deep --cwd '~/projects works as expected now

mkungla added a commit to mkungla/npm-check-updates that referenced this pull request Jan 20, 2021
@mkungla mkungla force-pushed the master branch 2 times, most recently from ce8e85c to fdd04c3 Compare January 20, 2021 17:38
@mkungla mkungla requested a review from raineorshine January 20, 2021 18:00
@mkungla
Copy link
Contributor Author

mkungla commented Jan 20, 2021

Since --deep is alias of default --packageFile glob then should it ignore other directories like test|tests etc.

@raineorshine
Copy link
Owner

Thanks for being responsive to feedback and helping to expand the functionality of npm-check-updates! This will benefit many other users.

@raineorshine raineorshine merged commit 76deaed into raineorshine:master Jan 20, 2021
@raineorshine
Copy link
Owner

Not sure what would be best option here... should versionmanager have some caching

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.

@mkungla This was WAY easier than I thought it would be! 0f81e6d

@mkungla
Copy link
Contributor Author

mkungla commented Jan 21, 2021

@mkungla This was WAY easier than I thought it would be! 0f81e6d

@raineorshine that's nice and to follow up

  1. Docs in README need also update.
  2. suppressErrors: false, should be removed so that errors like Error: EACCES: permission denied, scandir would not make it hang forever.

const qlobbyOts = {
suppressErrors: true,
ignore: ['**/node_modules/**']
}

@raineorshine
Copy link
Owner

  1. Docs in README need also update.

Yes, I forgot to run npm run build which regenerates the options in the README.

2. suppressErrors: false, should be removed so that errors like Error: EACCES: permission denied, scandir would not make it hang forever.

Great, done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement --deep flag to update deps in monorepo
3 participants