-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: update peerDependencies #4937
Conversation
"@loopback/boot": "^1.2.2", | ||
"@loopback/core": "^1.5.0", | ||
"@loopback/rest": "^1.10.3" | ||
"@loopback/boot": "^1.2.2 || ^2.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I would prefer to move away from peerDependencies
as it's really half-baked and not bullet-proof with npm
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss.
I introduced peer dependencies as a solution for two problems:
(1)
As an extension (e.g. @loopback/booter-lb3app
), I want to use the core packages provided by the target application (LoopBack project). I do not want to bundle my own copy of core packages.
Why is this important? It gives the target application full control of which version of core packages are used. I am not talking about version numbers - I want to allow users to use their own fork of a core package if they have a bug fix that is not a part of an official release (yet).
(2)
As an extension, I want to specify which versions of core packages I am compatible with. This way LoopBack users can pick the right versions and if they don't, then npm install
will warn them about a possible problem.
I am ok to use a different solution than peer dependencies as long as it preserves the properties of the current setup.
I see two discussion points:
- Do we want to continue using
peerDependencies
or not? - If yes, then what version ranges to use in peer dependencies? (Support only a single semver-major version or support as many versions as feasible?)
Let's keep this thread for discussing the first point and discuss the second point in https://github.com/strongloop/loopback-next/pull/4937/files#r395738578 below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
peerDependencies as it's really half-baked and not bullet-proof with npm.
From what I remember, peer dependencies used to be considered as deprecated in the past, but in the last few years the situation improved and they are no longer frowned upon. Popular tools like eslint
and babel
are using peer dependencies for their plugins.
$ npm info eslint-config-airbnb peerDependencies
{
eslint: '^5.16.0 || ^6.8.0',
'eslint-plugin-import': '^2.20.1',
'eslint-plugin-jsx-a11y': '^6.2.3',
'eslint-plugin-react': '^7.19.0',
'eslint-plugin-react-hooks': '^2.5.0 || ^1.7.0'
}
$ npm info @babel/preset-env peerDependencies
{
'@babel/core': '^7.0.0-0'
}
"@loopback/rest": "^1.10.3" | ||
"@loopback/boot": "^1.2.2 || ^2.0.0", | ||
"@loopback/core": "^1.5.0 || ^2.0.0", | ||
"@loopback/rest": "^1.10.3 || ^2.0.0 || ^3.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not very confident with the wide range as the CI only tests the module against latest versions of such dependencies.
The most reliable collection of @loopback/*
modules that are guaranteed to work together are these being released together :-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raymondfeng I think users will expect peer deps that meets the folowing criteria would work:
- Same major semver
- Equal or higher that min. version within semver (e.g.
@loopback/boot: ^1.2.2
)
If there's incompatibilities between 2 versions that meet the aforementioned criteria then we are breaking semver conventions.
So it may be a good idea to keep this range instead of pinning it to an exact version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@achrinza I meant to suggest we use "@loopback/rest": "^3.0.0"
. It's hard to ensure that this module is compatible with 1.x, 2.x, and 3.x without meaningful CI test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take our connectors as an example. They support both LB3 and LB4, therefore we have only one version to maintain for both LB3 and LB4 users.
I think it makes a lot of sense to adopt the same approach for our extensions once we start maintaining LB4 LTS versions (see #4398). This may include writing acceptance tests to verify that the current (master
) version of an extension works with older (LTS) versions of a core component, as @raymondfeng has mentioned in his comment.
However, we don't provide LTS for LB4 yet, therefore I think it's ok to use a narrow range for the peer dependencies for now. Especially considering that we don't have CI test coverage to verify compatibility with older versions. I don't think it would be a good use of our time to work on such CI setup now.
If we decide to limit the supported peer versions to the latest major version only, then can we please find a way how to automate the update process - ideally as part of our version release setup? I.e. when we publish a new version, then the peer dependencies should be updated to match the new versions created by the publishing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I withdraw my approval to favor #4972
I assume we're now switching to the #4972 solution? |
Close as #4972 is landed. We'll fix peer deps in next release (tentatively next week). |
Manually fixes peerDependency, maybe there will be a script to fix this automated in future.
Fixes #4889
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈