Handling package-lock.json for Horreum

[shivam-sharma7]

@johnaohara I don't know which node/npm version use by dependabot for generating PRs

[shivam-sharma7]

@johnaohara I don't know which node/npm version use by dependabot for generating PRs

This could be also a reason to get conflict in lock files

[johnaohara]

There are a number of issues that we have with package-lock.json that I think we need to resolve;

1 ) different version of npm produce different output, even within the same output "version"
2) contents of node_modules,
3) tab spacing - some PR's are single spaced, some are double spaced
4) conflict in package-lock between different PR's; This is tricky to sort out without someone losing the "race".
5) Uncertain which version of npm dependabot uses

When I get time later today, I will write up my thoughts on each point above. Please and more issues that you see

[shivam-sharma7]

Okay. I will be available at 7PM from my end

[shivam-sharma7]

@johnaohara let's meet today at google meet.

[johnaohara]

@shivam-sharma7 i dont have time today unfortunatley

[shivam-sharma7]

when you have time to do this? I have some question related Horreum

[johnaohara]

If we get this correct, there should not be many conflicts. The only time we will see conflicts is when 2 PRs make changes to the same or adjacent lines, which should be fairly rare.

Issues

1. different version of npm produce different output, even within the same output "version"

For this problem, as long as we use mvn clean install - which uses a version of node/npm that is fixed for the project, this should not be a problem. I think problems come about when someone executes npm i in horreum-web and a difference npm version is used by the shell.

2. contents of node_modules,

Looking at https://docs.npmjs.com/cli/v10/configuring-npm/package-lock-json - the contents of package-lock.json can change if the content of node_modules changes. So, we need to ensure that we run mvn clean -Premove-node-cache before running mvn install before creating the PR. If we checkout a different branch which has different versions, I think we could end up with an incorrect package-lock.json

3. tab spacing - some PR's are single spaced, some are double spaced

I honestly do not know what causes this, but it means the entire package-lock.json file it re-written (for example: https://github.com/Hyperfoil/Horreum/pull/1254/files#diff-7bd5bcb1bbc7060dbc4c33ca2164d862273f7518582df29baefca7f356f8daaa) . This causes a number of problems;

1 - You can not see what has changed, the entire file changes
2 - all subsequent PRs have conflicts as the diffs generated by git cover the entire file

I think we need to find the root cause of this and figure out how to resolve this problem

4. conflict in package-lock between different PR's; This is tricky to sort out without someone losing the "race".

This is always going to happen, but if we minimise the diffs, the likelihood of this occurring is greatly reduced

5. Uncertain which version of npm dependabot uses

It looks like the max version dependabot supports is npm v9, whereas in the project we use npmv10: https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file
We should look at how to resolve this.

Another thought, that we have kind of implemented already.

6. Floating dependency versions mean package-lock.json can update deps and transitive dependencies

If we lock the dependency version, the cascade of updates from dependencies and their transitive dependencies is greatly reduced.

Some other options:

not check package-lock.json

The other option is to not check package-lock.json into the master branch but, this mean we will need to;

a) Have some sort of automation to build/update package-lock.json for the stable branch, which is likely to have similar problems to the problems we have in master branch atm. I am reluctant to do this, because
b) change CI as it builds a prod version of the front end. Again, I am reluctant to do this because

Disable dependabot

We could decide to disable dependabot for front-end, but this would mean manual tracking and updating of dependencies. We would still have the same issue, but probably a lot less frequent as we would not be upgrading as often. I am reluctant to do this because upgrades become manual and we are more vulnerable to bugs/CVE's if we do not keep up-to-date

[stalep]

Thanks for the thorough analysis!
The "annoying" issue for me is when I'm working something that's not touching the frontend and I'm getting a conflict. Often it's very hard to see the diff of what has conflicted so you need to do a 'mvn clean install' and cross fingers....

[johnaohara]

I think this comes down to transitive dependencies ( which i think we should lock down) alongside large diffs being checked in/merged for package-lock.json - i.e. when the entire file has been changed previously - part of that I should check the scope of the changes, or we have a way to autogenerate the package-lock.json as part of the PR (but that kind of goes against the purpose of package-lock).

tbh I am not 100% sold on the way npm manages dependencies

[johnaohara]

but we need to come up with a workflow that works for us

[johnaohara]

Dependabot was working as expected : b45e1ca

Up until this commit: 0baef81

since then the spacing format of package-lock.json has been incorrect

[johnaohara]

I will re-commit package-lock.json with the correct spacing, and ensure that dependabot PR's are correctly formatted

[shivam-sharma7]

Working fine, I rebased my branch see 7b1b484

closing

[johnaohara]

I still have not fixed that problem :(

[shivam-sharma7]

Yes, This is happen again with dependabot https://github.com/Hyperfoil/Horreum/pull/1287/files#diff-7bd5bcb1bbc7060dbc4c33ca2164d862273f7518582df29baefca7f356f8daaa

[johnaohara]

yes, atm I am manually fixing the dependabot PRs ( I have a script) until I can find a proper solution

[johnaohara]

You can see this PR that I fixed: #1268

[shivam-sharma7]

Yes, I think locally its working fine?

[whitingjr]

@johnaohara Would it be sensible to enable prettier to in the Action workflow with npx prettier --check "package-lock.json" the json file. Failing any PR that does not meet the following formatting rules

{
  "tabWidth": 2,
  "useTabs": false
}

[whitingjr]

What's not tackled in the PR is when dependabot does reformat the json. Do we additionally want to adapt that Action workflow to re-write the file format to be compliant ?

[johnaohara]

i think it would be better to figure out why the package-lock.json produced by dependabot is incorrectly formatting the file and fixing that if we can

[whitingjr]

I checked. We cannot configure dependabot to format the generated package-lock.json. The recommended workaround for that is to create a workflow Action to reformat the file.

[johnaohara]

have you got a link to the recommendation?

[whitingjr]

Here's an example of a GitHub Actions workflow step that will reformat the package-lock.json using Prettier:

name: Check and auto-format

on: [pull_request]

jobs:
  prettier:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Set up Node.js
        uses: actions/setup-node@v1
        with:
          node-version: '16'
      - name: Install Prettier
        run: npm install --save-dev prettier
      - name: Check formatting
        run: npx prettier --check .
      - name: Reformat
        if: ${{ failure() }}
        run: |
          npx prettier --write package-lock.json
          git config user.name 'github-actions'
          git config user.email '[email protected]'
          git commit -am "style: format with Prettier"
          git push

[shivam-sharma7]

.prettierignore file can help us to achieve this by configure in horreum-web

[whitingjr]

@shivam-sharma7 to re-format the file contents ?

[whitingjr]

@shivam-sharma7 changing what happens locally is not an issue. Dependabot reformatting files is the main concern currently.

[shivam-sharma7]

@whitingjr We should report it

[whitingjr]

@shivam-sharma7 There is no ability to provide a prettifier configuration. Plus who are we to say the existing format is wrong. It is only wrong for our project because our configuration differs to the default.

[shivam-sharma7]

Yes agree! but this #1488 is needed in project I have seen many project used this. let me know what's wrong with this

[whitingjr]

@shivam-sharma7 Your PR #1488 affects local prettifier execution. But that isn't necessary. What we want to change is the execution of Dependabot when it proposes version bumps. Hence #1554 which reformats the file when it fails Horreum project prettifier rules.

[johnaohara]

We REALLY need to fix this issue, PR's are being merged that will cause conflicts e.g.: https://github.com/Hyperfoil/Horreum/pull/1524/files

[johnaohara]

I see this has been reverted, but we need to get this correct

[whitingjr]

@johnaohara #1554 is waiting for your review.