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

Remove appui-layout-react dep from map-layers and measure-tools #845

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

GintV
Copy link

@GintV GintV commented May 27, 2024

Addresses #835

Copy link
Contributor

@mdastous-bentley mdastous-bentley left a comment

Choose a reason for hiding this comment

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

This change has been marked as 'patch', I assume this wont break anything for our consumers ?

@GintV
Copy link
Author

GintV commented May 27, 2024

@mdastous-bentley We've been using both packages with AppUI 4.10+ without any issues for some time, so I guess not. Tho if not already, they will need to update to AppUI 4.10+ . I don't know it update of a peer dep should be considered minor release or not. I can update it to be "minor".

@mdastous-bentley
Copy link
Contributor

@mdastous-bentley We've been using both packages with AppUI 4.10+ without any issues for some time, so I guess not. Tho if not already, they will need to update to AppUI 4.10+ . I don't know it update of a peer dep should be considered minor release or not. I can update it to be "minor".

If it forces our users to upgrade their AppUI dependencies, this should be at be a minor changes.
@aruniverse Interested in your thoughts here

@saskliutas
Copy link
Member

Bumping peer dependency version is a breaking change because all consumers that use older version of that peer dependency breaks. So bump type should be set to major.

However, looking at the changes I don't see any imports form @itwin/appui-layout-react being changed so maybe it would be enough to remove @itwin/appui-layout-react from peer dependencies to resolve linked issue?

@GintV GintV changed the title Update map-layers and measure-tools to AppUI 4.10 Remove appui-layout-react dep from map-layers and measure-tools May 28, 2024
@GintV GintV enabled auto-merge (squash) May 28, 2024 10:43
@mdastous-bentley
Copy link
Contributor

Bumping peer dependency version is a breaking change because all consumers that use older version of that peer dependency breaks. So bump type should be set to major.

However, looking at the changes I don't see any imports form @itwin/appui-layout-react being changed so maybe it would be enough to remove @itwin/appui-layout-react from peer dependencies to resolve linked issue?

Good point, I'm wondering how we ended up having it in the peers.

GintV and others added 2 commits May 29, 2024 11:43
@raplemie
Copy link

raplemie commented Jun 7, 2024

Bumping peer dependency version is a breaking change because all consumers that use older version of that peer dependency breaks. So bump type should be set to major.
However, looking at the changes I don't see any imports form @itwin/appui-layout-react being changed so maybe it would be enough to remove @itwin/appui-layout-react from peer dependencies to resolve linked issue?

Good point, I'm wondering how we ended up having it in the peers.

It's a peer because it used to be part of the AppUI lockstep, they were all needed back then so they were peer deps.

So bump type should be set to major.

I'll have to disagree there, if you create a major each time you require newer peer dependency, we'll end up with version 125.10.96 within 2 year...

Any package that use this package would have to publish a new major everytime, which means that you will have AppUI update to new major on every time they want to use a new minor of iTwin.js, which means that this package will have to change major to use AppUI, and so on....

@grigasp
Copy link
Member

grigasp commented Jun 7, 2024

I'll have to disagree there, if you create a major each time you require newer peer dependency, we'll end up with version 125.10.96 within 2 year...

Any package that use this package would have to publish a new major everytime, which means that you will have AppUI update to new major on every time they want to use a new minor of iTwin.js, which means that this package will have to change major to use AppUI, and so on....

We hate this fact too, but this is still the rule, with the exception for packages that are released in a lockstep (itwinjs-core, appui).

@raplemie
Copy link

raplemie commented Jun 7, 2024

I'll have to disagree there, if you create a major each time you require newer peer dependency, we'll end up with version 125.10.96 within 2 year...
Any package that use this package would have to publish a new major everytime, which means that you will have AppUI update to new major on every time they want to use a new minor of iTwin.js, which means that this package will have to change major to use AppUI, and so on....

We hate this fact too, but this is still the rule, with the exception for packages that are released in a lockstep (itwinjs-core, appui).

Where is that rule defined ? I'd like more info on that if we start following this, because it means we will get stuck with old dependency. Moving to iTwinUI already have been slow, if we start doing that for every single packages that are reused, the cascade will be unmanageable because they all are supposed to be "Major" changes, where the only change will be "we are now using this new hook in a widget."

@saskliutas
Copy link
Member

I'll have to disagree there, if you create a major each time you require newer peer dependency, we'll end up with version 125.10.96 within 2 year...
Any package that use this package would have to publish a new major everytime, which means that you will have AppUI update to new major on every time they want to use a new minor of iTwin.js, which means that this package will have to change major to use AppUI, and so on....

We hate this fact too, but this is still the rule, with the exception for packages that are released in a lockstep (itwinjs-core, appui).

Where is that rule defined ? I'd like more info on that if we start following this, because it means we will get stuck with old dependency. Moving to iTwinUI already have been slow, if we start doing that for every single packages that are reused, the cascade will be unmanageable because they all are supposed to be "Major" changes, where the only change will be "we are now using this new hook in a widget."

You can read about peer dep bumping at this discussion in semver repo. semver/semver#502

@raplemie
Copy link

raplemie commented Jun 7, 2024

You can read about peer dep bumping at this discussion in semver repo. semver/semver#502

I can read the discussion, but these are random opinions as far as I am concerned, not "A rule" of any kind. If you update your dependencies, you update your dependencies. Most packages arent built with statics all over the places that causes them to require a unique lockstep. OR, we need to start to be much more focused on this, and whenever a new feature is added in any of the core packages, EVERY bentley packages needs to be updated within a week, in the right order, so each other package will be able to update once to account for all these changes, which does not make any sense :)

Because I can update my package to use the new iTwin.js package, (major update) then appui follows up and happen to require this iTwin.js (major update 2), then presentation components updates to this new AppUI stuff (major update 3), then measure tools also require this new iTwin.js version (major update 4)

I can understand for Major peer deps dependency, but for minor... Not so much.

@raplemie
Copy link

raplemie commented Jun 7, 2024

I'm being confirmed by @calebmshafer that this is how we want to go, so be it. Can't wait to see the results.

@grigasp
Copy link
Member

grigasp commented Jun 7, 2024

I'm being confirmed by @calebmshafer that this is how we want to go, so be it. Can't wait to see the results.

You don't need to bump peer dependencies that often. You can still bump the dev dependency and see the new APIs, but just have to be careful to check if they actually exist.

@raplemie
Copy link

raplemie commented Jun 7, 2024

"Making sure that it exists" is not a simple thing, because none of the packages mentions when their new API is introduced... So that means that whenever I code something new, using any API that is not clearly already used in my code, we need to make the investigation as to "did this exists all the way up to when my dependency is listed", and if not, make an alternate code path for this. Then, when we fix an issue by using a new API, the patch note would utimately need to say: "We fix ..., but be sure to use package X version ... to actually see the fix".

Edit: (The fact that we dont know easily when something was introduced is already an issue regardless of this peerDeps
discussion, but making "multiple" code path in each case is what this is adding to that burden)

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

Successfully merging this pull request may close these issues.

6 participants