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

build: no strict version for kuzzle-sdk #2537

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

sebtiz13
Copy link
Member

@sebtiz13 sebtiz13 commented Jun 5, 2024

What does this PR do ?

Change kuzzle-sdk version restriction to allow npm to dedup the package when it's required in other package.

Explanation

Currently, Kuzzle require strictly [email protected], if a package like plugin require kuzzle and [email protected]

With strict restriction, NPM produce this install

With package restriction like kuzzle-sdk@^7.11.3 (so accept greater than 7.11.3 but less than 8), NPM produce this install

Here, kuzzle-sdk is not installed in kuzzle directory because [email protected] satisfy the restriction.
Also, normally in future if I want to install [email protected] in my package, npm will throw an error for incompatibility (need to verify, because npm haven't consistent behavior through the versions)

@sebtiz13 sebtiz13 self-assigned this Jun 5, 2024
@sebtiz13 sebtiz13 force-pushed the build/improve-dependencies branch from 7d7b458 to 1073a04 Compare June 6, 2024 08:29
Copy link

sonarqubecloud bot commented Jun 6, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@rolljee
Copy link
Contributor

rolljee commented Jun 6, 2024

What does this PR do ?

Change kuzzle-sdk version restriction to allow npm to dedup the package when it's required in other package.

Explanation

Currently, Kuzzle require strictly [email protected], if a package like plugin require kuzzle and [email protected]

With strict restriction, NPM produce this install

With package restriction like kuzzle-sdk@^7.11.3 (so accept greater than 7.11.3 but less than 8), NPM produce this install

Here, kuzzle-sdk is not installed in kuzzle directory because [email protected] satisfy the restriction. Also, normally in future if I want to install [email protected] in my package, npm will throw an error for incompatibility (need to verify, because npm haven't consistent behavior through the versions)

Wouldn't it be a best practice to update the sdk to his latest version but still left fixed to avoid issue ?

Like we could launch an action that launch ncu and update if all tests aren't failing ?

@sebtiz13
Copy link
Member Author

sebtiz13 commented Jun 7, 2024

What does this PR do ?

Change kuzzle-sdk version restriction to allow npm to dedup the package when it's required in other package.

Explanation

Currently, Kuzzle require strictly [email protected], if a package like plugin require kuzzle and [email protected]
With strict restriction, NPM produce this install

With package restriction like kuzzle-sdk@^7.11.3 (so accept greater than 7.11.3 but less than 8), NPM produce this install

Here, kuzzle-sdk is not installed in kuzzle directory because [email protected] satisfy the restriction. Also, normally in future if I want to install [email protected] in my package, npm will throw an error for incompatibility (need to verify, because npm haven't consistent behavior through the versions)

Wouldn't it be a best practice to update the sdk to his latest version but still left fixed to avoid issue ?

Like we could launch an action that launch ncu and update if all tests aren't failing ?

The problem it's if we use the version 7.11.1 in the core and then we have a 7.11.3 update. We need to release the core only to update the package version. Instead, with not strict restriction, It's not mandatory to release the core only for that.
It's for this reason we restrict the version range of package with caret ^ to accept only from 7.11.1 to 8 (not include) to ensure the compatibility (in theory no breaking change), but we can be stricter and use tilde to restrict the range only on patch so from 7.11.1 to 7.12 (not include) to be safer.

@rolljee
Copy link
Contributor

rolljee commented Jun 10, 2024

What does this PR do ?

Change kuzzle-sdk version restriction to allow npm to dedup the package when it's required in other package.

Explanation

Currently, Kuzzle require strictly [email protected], if a package like plugin require kuzzle and [email protected]
With strict restriction, NPM produce this install

With package restriction like kuzzle-sdk@^7.11.3 (so accept greater than 7.11.3 but less than 8), NPM produce this install

Here, kuzzle-sdk is not installed in kuzzle directory because [email protected] satisfy the restriction. Also, normally in future if I want to install [email protected] in my package, npm will throw an error for incompatibility (need to verify, because npm haven't consistent behavior through the versions)

Wouldn't it be a best practice to update the sdk to his latest version but still left fixed to avoid issue ?
Like we could launch an action that launch ncu and update if all tests aren't failing ?

The problem it's if we use the version 7.11.1 in the core and then we have a 7.11.3 update. We need to release the core only to update the package version. Instead, with not strict restriction, It's not mandatory to release the core only for that. It's for this reason we restrict the version range of package with caret ^ to accept only from 7.11.1 to 8 (not include) to ensure the compatibility (in theory no breaking change), but we can be stricter and use tilde to restrict the range only on patch so from 7.11.1 to 7.12 (not include) to be safer.

If I understand in this case,

Does it mean in the case we have kuzzle requesting the 7.11.1 and another repo is requesting 7.11.3
It will install the 7.11.3 right ?

So it also mean that kuzzle will too run in 7.11.3 even tho it as only be tested with the 7.11.1 ?

I think I am okay with it, but I just want to be fully aware of all that :)

@alexandrebouthinon alexandrebouthinon merged commit 87be8bd into 2-dev Jun 10, 2024
35 checks passed
@alexandrebouthinon alexandrebouthinon deleted the build/improve-dependencies branch June 10, 2024 13:10
@kuzzle
Copy link
Contributor

kuzzle commented Jul 22, 2024

🎉 This PR is included in version 2.31.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kuzzle
Copy link
Contributor

kuzzle commented Jul 22, 2024

🎉 This PR is included in version 2.31.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kuzzle kuzzle added the released This issue/pull request has been released. label Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @beta released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants