Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

Update the "welcome" package's package-lock.json (eliminates some warnings in CI)#21887

Merged
sadick254 merged 1 commit intoatom:masterfrom
DeeDeeG:update-package-lock-for-welcome-package
Jan 25, 2021
Merged

Update the "welcome" package's package-lock.json (eliminates some warnings in CI)#21887
sadick254 merged 1 commit intoatom:masterfrom
DeeDeeG:update-package-lock-for-welcome-package

Conversation

@DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Jan 22, 2021

Requirements for Contributing a Bug Fix (from template, click to expand):

Identify the Bug

There were warnings about cache keys changing in the macOS Tests packages-2 job. (Follow-up to #21884).

Description of the Change

I ran npm install (and apm install -- the result was the same) in the packages/welcome folder, and committed the updated lockfile.

No actual dependencies or dependency versions were updated. Just the "requires" fields and some "optional" fields.

(I'm not sure why every "requires" field had been recorded with absolute version numbers... Modern npm and apm should record the actual semver range for each dependency in each package's respective package.json file... Not the absolute, resolved/final version number.)

Alternate Designs

This lockfile was updated when installing its test runner dependencies during script/test. I tried running apm ci instead of apm install to install these dependencies. That would be more consistent and slightly faster. The problem with that is, some of the packages with test runner dependencies do not ship lockfiles. And you can't run npm ci/apm ci without a lockfile already being present.

This alternate design would be doable if we went through all of the packages in Atom core that have test runners specified in their package.json files, and made sure each of these packages ship a package-lock.json file. However, committing a lockfile to a repo is a subjective choice with some tradeoffs. There are times when not having a lockfile is legitimate. So I would rather not enforce every core package have a lockfile just to get rid of these warnings in CI.

Possible Drawbacks

None.

Verification Process

I did a CI run; the warnings about The given cache key has changed . . . went away.
https://dev.azure.com/DeeDeeG/b/_build/results?buildId=1019

Release Notes

N/A

No actual dependencies or dependency versions were updated.
Just the "requires" fields and some "optional" fields.

This should make welcome's package-lock.json stable
through subsequent runs of npm install or apm install.

(I'm not sure why every "requires" field had been recorded with
absolute version numbers... Modern npm and apm should record
the actual semver range for each dependency in each package's
respective package.json file...
Not the absolute, resolved/final version number.)
@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Jan 23, 2021

@sadick254 this eliminates the warning messages in CI I introduced in #21884.


Explanation: I figured out that one of the packages had an outdated format of data in package-lock.json. Now that that's updated (no dependencies or versions changed, just the way it's recorded) the file doesn't change after an apm install, and the cache ID is stable, so the warnings go away.

Copy link
Contributor

@sadick254 sadick254 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @DeeDeeG for the quick follow up PR. This looks great. 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants