iTwin
Remove appui-layout-react dep from map-layers and measure-tools
@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 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
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?
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-reactbeing changed so maybe it would be enough to remove@itwin/appui-layout-reactfrom peer dependencies to resolve linked issue?
Good point, I'm wondering how we ended up having it in the peers.
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-reactbeing changed so maybe it would be enough to remove@itwin/appui-layout-reactfrom 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....
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).
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."
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. https://github.com/semver/semver/issues/502
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.
I'm being confirmed by @calebmshafer that this is how we want to go, so be it. Can't wait to see the results.
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.
"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)
I think we can close this PR, measure-tools was updated in https://github.com/iTwin/viewer-components-react/pull/1097 And map-layers will be updated shortly in a separate pr
map-layers pr: https://github.com/iTwin/viewer-components-react/pull/1136