obsidianmd / obsidian-sample-plugin

2.61k stars 924 forks source link

Stuck build, many plugins affected. Any reason to gitignore package-lock.json? #13

Closed gavvvr closed 2 years ago

gavvvr commented 3 years ago

The package-lock.json is not included to git repo by intention. What is the reason? If you want stable build, you usually want to check in such lock-files to VCS.

Today I faced the problem with my Imgur plugin based on this template. My build has stuck. That's exactly because package-lock.json is not committed. The package.json is not enough, because it does not pin exact version of dependencies. With the ^-notation (example: "typescript": "^4.2.4") it allows actual minor and patch versions to be higher on npm install if newer version of the dependency is available on npm registry (see npm docs).

The build of this plugin's template is currently broken too. It will stuck forever in the very end after:

...
main.ts → ....
created . in 1.4s
# stuck here

Other plugins generated from this template and not having lock-file in git are affected too, i.e.:

I do not know the root cause of the problem. But from my experiments, the culprit of the stuck build with this Rollup setup is TypeScript 4.4.1+ which gets installed now on npm install without package-lock.json.

I think that the lock-file should not be ignored and must be committed to git. Also another common mistake I see people make is: using npm install instead of npm ci on their automated builds. npm install leaves a possibility for lock-file to be updated on clean install if the situation on npm registry has changed. I think the right choice for CI should be the npm ci command with lock-file tracked by vcs to get stable reproducible builds.

lishid commented 3 years ago

Perhaps we should pin the versions? I've specifically excluded package-lock.json because it's very finnicky and tends to get updated all the time. Internally, we pin all of our dependencies with package.json, but not the entire sub-dependencies tree.

joethei commented 3 years ago

From my research this seems to be the main culprit: https://github.com/rollup/rollup/issues/4213

gavvvr commented 3 years ago

Hi @lishid

I've specifically excluded package-lock.json because it's very finnicky and tends to get updated all the time

This does not sound like a serious reason to ignore it. It is designed that way and the lock-file usually changes together with package.json (For me it is not: all the time, it changes as expected) or whenever you are ready to upgrade your dependencies. All the other time npm ci is the best choice, which won't edit your lock-file and will guarantee a consistent build and dependencies resolution.

Hard-pinning dependencies manually in package.json is an option, but looks more like a workaround. I see 2 disadvantages:

My vote goes for committing lock-file to git


@joethei, thanks for sharing. I think you are right, this is the correct issue

lishid commented 2 years ago

Should have been fixed by https://github.com/obsidianmd/obsidian-sample-plugin/commit/3afc9d78ab809784d8ccf6d17e94c1509330e217