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

Leaving versions of dependencies packages unspecified may lead to surprising behaviour for downstream users #26

Open
rpuboh opened this issue May 19, 2024 · 6 comments

Comments

@rpuboh
Copy link

rpuboh commented May 19, 2024

Currently Awesome Gadgets serves as a template package for downstream users to develop on. Users will probably add dependencies to package.json on top what this repo itself has, as shown in https://github.com/qiuwenbaike/QiuwenGadgets/blob/eda7b887ef188662b20b222d7352c8ecc9e762e2/package.json#L48

Since downstream users always modify the list of dependencies, the pnpm-lock.yaml shipped with this package would be invalidated. pnpm will re-resolve the dependencies during pnpm i, during which packages with version unspecified (latest) will be updated to the latest version (e.g. types-mediawiki-renovate

"types-mediawiki-renovate": "latest",
). Such behaviour is surprising and undesired for end users, because their build process can mysteriously break after adding a benign new dependency. Users even can't fix the breakage by reverting changes to package.json (pnpm-lock.yaml before the invalidation is needed to avoid updating dependencies to incompatible versions).

This issue can be demonstrated by downloading a previous version (4.6.0) of Awesome Gadgets. Then adding random package to package.json (say left-pad) will cause pnpm i to warn about a conflict in resolving dependencies. It may also cause more subtle breakage that only surfaces during build process, for example updated type definitions from types-mediawiki-renovate can become incompatible with existing gadgets source code.

Fixating the version of every dependencies in package.json might be a bit overkill (though it's almost done this way already). Nevertheless, at least the dependencies that are more likely to cause compatibility problems shall have their versions explicitly specified in package.json. This also helps people make sense of what is actually updated in a new release that only updates versions of dependencies. Although pnpm-lock.yaml is tracked in git, its diff is often too verbose to make sense of.

@AnYiEE
Copy link
Owner

AnYiEE commented May 20, 2024

What changes do you expect?

  • Do not track changes to pnpm-lock.yaml, generate it on demand by downstream.
  • Switch packages specified as "latest" to more specific versions.

For conflicts in pnpm-lock.yaml, I have set up a git hook that automatically patches pnpm-lock.yaml after a merge. It seems like it's not working? I think this is the root of the problem.

@rpuboh
Copy link
Author

rpuboh commented May 20, 2024

It may help to consider what kind of "contract" do you want to maintain with users that stay at a particular version. Users should be assumed to add/delete dependencies in package.json often (triggering pnpm recalculation each time). I feel that some packages integral to the build environment (like type definitions) should not change in a release of Awesome Gadgets. You may consider specifying a fixed version specifier (without ^, ~ or other semver operators). However I admit that I don't have enough experience with frontend development to draw the line for it.

post-merge hook is triggered after the merge commit (https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks#_other_client_hooks), therefore it does not help with resolving the conflict during a git merge.

Tracking pnpm-lock.yaml in version control makes sense for Awesome Gadgets itself, but it's useless for downstream users (due to modifications to dependencies list). I'm not sure what's the best practice here. I notice that pnpm has a git-branch-lockfile feature (so downstream users can use a different pnpm-lock-xxx.yaml) but it might not be relevant here.

@rpuboh
Copy link
Author

rpuboh commented May 20, 2024

If users want each release of Awesome Gadgets to behave like stable releases of Linux distributions (only bug and security fixes, no new feature that can break compatibility), then all the dependency version specifiers should be fixed to a version or use ~. For the ~ case the user has to trust the developers of other packages to properly follow semver semantics. ^ sounds too risky to me.

However it should be noted that the dependencies of dependencies may have looser specifiers even if the direct dependencies have their version fixed. Maybe the document should anyway warn users that dependencies may implicitly update and cause breakage. I'm not familiar with js development, and I wonder if this is common sense among js developers.

@AnYiEE
Copy link
Owner

AnYiEE commented May 22, 2024

You can refer to this for specifications on version numbers. I don't consider changes in types as a "breaking change". In a sense, the "types-mediawiki-renovate" package is already part of this repository.

@rpuboh
Copy link
Author

rpuboh commented May 22, 2024

I don't object to your last sentence, and I acknowledge that fixing type declarations is probably not a big task (that sounds "breaking").

However, given that types-mediawiki-renovate is part of Awesome Gadgets, why not fixate its version in package.json so it won't be out of sync with the version of Awesome Gadgets (when users invalidate the lock file)? I notice that version 4.6.2 (32892ad) is such kind of a release that's made just because of new types-mediawiki-renovate. You probably do not intend users to use [email protected] with [email protected].

AnYiEE added a commit that referenced this issue May 30, 2024
build: update dependencies
see #26 for more information
@rpuboh
Copy link
Author

rpuboh commented May 30, 2024

By the way, I just notice that git allows supplying a custom "merge driver", and pnpm has one for the lock file (https://github.com/pnpm/merge-driver, however I suggest checking also https://github.com/npm/npm-merge-driver, which seems better documented).

You may consider using it for .github/workflows/sync.yml (to avoid repeated CI failure notification due to lock file conflict). I'm not sure if it makes sense to deploy this also to developers' environment.

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

No branches or pull requests

2 participants