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

[measure-tools] update to itwinui v3 #1097

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

Conversation

aruniverse
Copy link
Member

@aruniverse aruniverse commented Nov 22, 2024

  • update to itwinui v3
    • only being used for 1 icon
  • need to update appui peer to minimum of 4.10 to use itwinuiv3 in appui so updated to the latest
    • this allows us to drop appui-layout entirely
  • updated core, presentation, appui deps to latest minor to make sure no apis were deprecated in devDeps
  • updated to support react 18

"@itwin/appui-abstract": "^4.10.0",
"@itwin/appui-react": "^4.17.0",
"@itwin/components-react": "^4.17.0",
"@itwin/core-bentley": "^4.10.0",

Choose a reason for hiding this comment

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

@aruniverse Since these are peerDependencies, shouldnt that cause this to be a major change ? Basically it means that if we want to use this new version, we need to bump our dependencies to consume it and make sure that we are to the latest dependencies, which if no code change is required, means that there is no real reasons to change the iTwin.js dependencies. (on top of that, I think it shouldnt be hidden under the iTwinUI V3 changes)

It would at least make this 1.0.0, and make this package more easily manageable with versions, rather than 0.x version which is ignored by package management.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per my other comment, if there are no new APIs being used, the peer deps should probably just remain the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats very fair, while these are "breaking" since this pkg is 0.x its fair imo.
I do not want to bump this to 1.0 until we have clear idea of ownership of this pkg, also with both appui and core majors around the corner we'd have to do a 2.0 and 3.0 release shortly back to back.

I reverted bumping the core peer deps. The AppUI you need to bump to a minimum of 4.10 to pull in iTwinUI@3. And at that point if youre bumping it, might as well go to the latest. This also helps us verify we're not using any deprecated APIs that will be removed in AppUI@5.

DevDeps have been updated to latest minors and that should be fine

@Maxime-Brassard
Copy link
Contributor

I'm unsure here why there was a need to change the peerDeps at all. Since there's no new api that are being used.
Are all of these package updates (peerDeps and devDeps) really needed in that case?
I would not want every consuming app for this package to have to update all of this right away.

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.

3 participants