Skip to content

Stick to package-lock.json in actions#4032

Merged
max-ostapenko merged 4 commits intomainfrom
important-cattle
Mar 6, 2025
Merged

Stick to package-lock.json in actions#4032
max-ostapenko merged 4 commits intomainfrom
important-cattle

Conversation

@max-ostapenko
Copy link
Copy Markdown
Contributor

@max-ostapenko max-ostapenko commented Mar 5, 2025

Related to #4021 (comment)

@max-ostapenko max-ostapenko requested a review from tunetheweb March 5, 2025 21:41
@tunetheweb
Copy link
Copy Markdown
Member

Interesting idea, but I wonder if we shouldn't just remove package-lock.json from git instead?

Because at the moment I'll still generate package-lock.json diffs just by doing npm install locally if any of the dependencies change. This could be particularly confusing (and cause merge conflicts!) for less technical people editing the chapters.

Normally it's nice to have package-lock.json in git to ensure stabilty of packages and controlled updates. But since this isn't use to run the site in production (as it runs on python and contains all the JavaScript-generate files on deployment already) we can probably live without that for ease of maintenance of package updates.

@max-ostapenko
Copy link
Copy Markdown
Contributor Author

I got a bit confused, misguided by this short description.
So npm install will actually update package-lock.json.

With wildcards now, I wouldn't like letting actions going wild.
So I'd prefer using npm ci in actions.

@max-ostapenko max-ostapenko changed the title Ignore package-lock for website tests Stick to package-lock.json in actions Mar 5, 2025
@tunetheweb
Copy link
Copy Markdown
Member

That's quite nice. But still doesn't solve the autoupdating of package-lock.json issue by other contributors when using wildcard version numbers.

Say we have the following situation:

  • Package 1 releases a new minor version
  • Dependabot no longer opens a PR since we use wildcards in package.json
  • Contributor 1 submits a typo fix to our of our markdown files. They have run npm install (or npm run start) so automatically include a new package-lock.json file in the PR.
  • For whatever reason the PR review isn't actioned immediately
  • Package 1 releases another new minor version.
  • Contributor 2 submits a typo fix to our of our markdown files. They have run npm install (or npm run start) so automatically include a new package-lock.json file in the PR.

We now have a merge conflict to deal with. Which can definitely be confusing for package-lock.json files (since they are often somewhat complex changes, rather than one line changes) and since some of our markdown contributors aren't that proficient in git.

Not using wildcards and using dependabot avoids this. But at the cost of noise (having lots of commits).

So options are:

  • Live with this merge conflict potential and/or ask contributors to exclude package-lock.json updates in their PRs.
  • Don't include package-lock.json to git. Slight risk of version incompatibility.
  • Don't use wildcards in version dependencies.

No matter what we do here, making the change in this PR is probably good anyway to prevent dependence updates in CI for dependent packages not included in package.json, but I still don't think it enables wildcard usage in package.json.

@max-ostapenko
Copy link
Copy Markdown
Contributor Author

Ok, so it's not particularly the CI issue, but PR workflow may degrade.
I had package-lock.json conflicts quite often even without wildcards, but it's probably because of working with several branches simultaneously.
Will note why it happens, and if there is an argument to exclude package-lock.json.
For now let's keep it and remove wildcards.

But I'd still prefer to have only impactful dependabot PRs.
I found a 3rd option to adjust dependabot config instead to skip patch updates for a group of dependencies that have frequent updates?
This should be the safest option. WDYT?

@tunetheweb
Copy link
Copy Markdown
Member

Yeah could do that.

I wonder if it's possible to auto-merge dependabot PRs if CI passes without issue? Will still get some email noise, but at least don't need to manually action them then?

But either way this PR is good to merge (as long as we are keeping package-lock.json in git).

@max-ostapenko
Copy link
Copy Markdown
Contributor Author

Exactly, I'd prefer to keep emails actionable.

So I'll merge this and will rollback wildcards if we see it causing conflicts.

@max-ostapenko max-ostapenko merged commit f7f6c1b into main Mar 6, 2025
@max-ostapenko max-ostapenko deleted the important-cattle branch March 6, 2025 22:54
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.

2 participants