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

Support node_modules that live in source control #13926

Open
dominykas opened this issue Jan 31, 2022 · 6 comments
Open

Support node_modules that live in source control #13926

dominykas opened this issue Jan 31, 2022 · 6 comments
Labels
help wanted Help is needed or welcomed on this issue manager:npm package.json files (npm/yarn/pnpm) priority-4-low Low priority, unlikely to be done unless it becomes important to more people type:feature Feature (new functionality)

Comments

@dominykas
Copy link
Contributor

dominykas commented Jan 31, 2022

What would you like Renovate to be able to do?

Having node_modules checked into source control is not a very common pattern, however there is at least one use case where it simplifies life a lot: creating Github Actions. GH expects all the things needed for runtime to be in the repository, so as a developer you're left with either checking in the node_modules or using a bundler.

The feature request is two-fold:

  1. The node_modules need to be wiped before renovation:
    • Without wiping, when used with a lockfile1, npm does not have the information about installed package meta data in the cache and so when it tries to recreate the shrinkwrap it does not have the checksums2. See an example resulting lock file maintenance PR: chore(deps): lock file maintenance pkgjs/action#31.
    • Moreover, when the node_modules are present, npm will not install the latest versions of the packages if the existing ones match the version range (unless npm update is run), so this effectively disables the core purpose of renovate.
  2. Need to push the updated node_modules back into source control after the upgrade.

If you have any ideas on how this should be implemented, please tell us here.

There's probably several considerations for this feature:

  • Does this need a separate config option or can it be implied (based on the presence of node_modules)?
  • Does this need separate config options for both of the steps - wiping node_modules at the start and adding them back at the end?
  • When lock files are not used, does this need a separate "node_modules maintenance" (which should probably run on a similar schedule as lock file maintenance?)

Alternatively, this could maybe become a little bit more abstract, not tied to node_modules, and just have separate config options for "wipe these files/folders at the start" and "check these folders/files into source control when done".

Is this a feature you are interested in implementing yourself?

Yes

Footnotes

  1. lock files are not strictly necessary when checking node_modules in, but it is one way to ensure that the whole dependency tree receives updates via renovate.

  2. one could argue that this is an issue with npm, however we have to keep in mind, that if npm did not fetch and unpack the files, then it can't really know or calculate the checksum of the unpacked files, therefore leaving it out of the lockfiles is quite possibly the correct behavior.

@dominykas dominykas added priority-5-triage status:requirements Full requirements are not yet known, so implementation should not be started type:feature Feature (new functionality) labels Jan 31, 2022
@viceice
Copy link
Member

viceice commented Jan 31, 2022

Duplicate of #13674

@viceice viceice marked this as a duplicate of #13674 Jan 31, 2022
@viceice viceice closed this as completed Jan 31, 2022
@dominykas
Copy link
Contributor Author

Shall I add the detail about wiping node_modules before kicking off the upgrade in #13674?

@rarkins rarkins reopened this Feb 1, 2022
@rarkins
Copy link
Collaborator

rarkins commented Feb 1, 2022

I think this one sounds a bit specialist - especially the node_modules wiping - so let's keep it as a separate feature discussion.

We already support "vendored" packages in other manages such as gomod, bundler, yarn (cache), and composer, if my memory is correct. I think merely adding/updating the node_modules is a simple part. Making sure you wipe node_modules at the right times is perhaps another concept which could exist independently of node_modules committing, because it can have an impact on the lock file contents too.

@rarkins rarkins added manager:npm package.json files (npm/yarn/pnpm) priority-4-low Low priority, unlikely to be done unless it becomes important to more people auto:reproduction A minimal reproduction is necessary to proceed and removed priority-5-triage labels Feb 1, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2022

Hi there,

Help us by making a minimal reproduction repository.

Before we can start work on your issue we first need to know exactly what's causing the current behavior. A minimal reproduction helps us with this.

To get started, please read our guide on creating a minimal reproduction to understand what is needed.

We may close the issue if you (or someone else) have not provided a minimal reproduction within two weeks. If you need more time, or are stuck, please ask for help or more time in a comment.

Good luck,

The Renovate team

@dominykas
Copy link
Contributor Author

Repo to reproduce the issue: https://github.com/dominykas/test-renovate-13926

Build log: https://app.renovatebot.com/dashboard#github/dominykas/test-renovate-13926/568141763

This does not demonstrate the missing integrity fields (I'll revisit that later), but it does demo the failure to upgrade deps (lockfile has debug@4.3.2, but there is a 4.3.3 already available - this is because npm install is happy to not make any changes to the existing node_modules).

@dominykas
Copy link
Contributor Author

And the integrity part reproduced itself: https://github.com/dominykas/test-renovate-13926/pull/5/files

@rarkins rarkins added reproduction:provided help wanted Help is needed or welcomed on this issue and removed auto:reproduction A minimal reproduction is necessary to proceed status:requirements Full requirements are not yet known, so implementation should not be started labels May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Help is needed or welcomed on this issue manager:npm package.json files (npm/yarn/pnpm) priority-4-low Low priority, unlikely to be done unless it becomes important to more people type:feature Feature (new functionality)
Projects
None yet
Development

No branches or pull requests

3 participants