Adjust to iTwinUI V3 breaks. #113

raplemie · 2023-12-08T19:49:56Z

Breaking changes...

With the previous release, there were in fact, breaking changes in that the iTwinUI styles are no longer applied at all when using our package as people did before... Since we already released the breaking change as a Minor, I simply put this as a patch...

Add `iTwinUI` V3 style import info in readmes.

In 2 days, I got 3 questions as to why everything was broken, so I think updating the readme is a must, so I can at least direct people somewhere.

Fix `useToaster` missing `ThemeProvider` wrapping.

To be sure that everything was working as expected, I swapped the storybook itwin react back to 2 and tested... the results were not great, turns out that useToaster hook actually require the component itself to be in a ThemeProvider, so this:

function Component() {
 const toaster = useToaster();
 return <ThemeProvider><Button onClick={() => toaster.positive("message")}/></ThemeProvider>;
}

do not work, Component need to be in a ThemeProvider so the hook is accessible within it *(basically, it access a context set in ThemeProvider...)

Switch some `iui` classes to their component version.

We still had a place were we were using directly classes, these are not defined if we use V2/V3 (not sure they were in V3 only, didnt test)
So I replaced iui-input-container classes by <InputGrid> component, which I think is what was intended.
Same thing for iui-anchor vs <Anchor>;

Removed default `theme="inherit"` from `ThemeProviders`

inherit is the default value for ThemeProvider in V3, so we dont have to pollute our code with them :)

Remaining issue

~~The theme will not sync with V2 ThemeProvider, only take the value at mount, I filed an issue with iTwinUI regarding this: iTwin/iTwinUI#1730~~ This was released and now included in this PR with the inclusion of iTwinUI react 3.1.0, we are now also using iTwinUi v2 in storybook to have a similar setup than applications.

Add `iTwinUI` V3 style import info in readmes. Switch some `iui` classes to their component version. Removed default `theme="inherit"` from `ThemeProviders`

veekeys · 2023-12-11T10:49:27Z

packages/modules/create-imodel/src/components/create-imodel/CreateIModel.tsx

@@ -78,6 +78,14 @@ export type CreateIModelProps = {
 };

 export function CreateIModel(props: CreateIModelProps) {
+  return (
+    <ThemeProvider>


sure its needed here? base component has it already at the root. what does this do? is it for toaster?
lets remove then from base theme provider?

This is needed because of our use of useToaster in the original component, since these component can be used directly, they need to be wrapped. Now I question the need of wrapping our component at all since the styles need to be imported by the application anyway, they already need to add the potential complexity of importing aliased pacakge... We could advertize ourselves as "Like iTwinUI component, our components need to be wrapped in a iTwinUI V3 ThemeProvider". I dont like delegating our problems to the user, but since this is how iTwinUI must be used, we might be adding more issues by duplicating ThemeProviders like we do for these components.

i think i agree with the idea of removing ThemeProvider 🤔

in v2, it was needed because packages were creating "boundaries", but v3 doesn't have that problem (v2 and v3 can overlap) so it should be fine to make the application responsible for it.

this whole update should probably have gone into admin-components v2 though.

however this may not be realistic. i don't think react context works once you use an alias, because it is a "different" package.

Everyone would likely need to have the import and a rename, on top of v2:

import { ThemeProvider } from "@itwin/itwinui-react"; import {ThemeProvider as ThemeProviderV3 } as "itwinui-react-v3"; import "itwinui-react-v3/styles.css"; <ThemeProvider theme={theme}> <ThemeProviderV3 theme={theme}> <App /> </ThemeProviderV3> </ThemeProvider>

As these packages are different, the context should work as they are imported from their packages. (this would also work around 1730, but make using our package even more complex.)

The main issue regarding this is the current percieved issue by applications of using the theme provider from V3 will cause issue to their otherwise V2 applications. So I'm not sure we are ready to move towards this.

lets remove then from base theme provider?

Yes, I didnt notice before that Base was not exported in the barrell and I thought it was also meant to be used directly, I'll remove it from Base.

As these packages are different, the context should work as they are imported from their packages.

What I mean is that the following are imported from "different" packages:

// in admin-components: "@itwin/itwinui-react": "^3" import { ThemeProvider } from "@itwin/itwinui-react";

// in application: "itwinui-react-v3": "npm:@itwin/itwinui-react@3" import { ThemeProvider as ThemeProviderV3 } from "itwinui-react-v3";

So they will likely not use the same context, i.e. ThemeContext inside itwinui-react-v3 is different from the ThemeContext inside @itwin/itwinui-react, because they are not the same module.

But I haven't tested, so this should be verified.

veekeys · 2023-12-11T10:50:15Z

packages/modules/create-imodel/src/components/upload-image/UploadImage.tsx

-export function UploadImage({
+export function UploadImage(props: UploadImageProps) {
+  return (
+    <ThemeProvider>


wow..
this is just bad with extra divs randomly..
@mayank99 Can this be fixed?

can what be fixed? you don't need ThemeProvider in every single component, you only need it once per tree.

if all components are separate/standalone entrypoints, then this might just be the best way.

@veekeys I see that this component now uses the useIModelComponent hook which makes this component essentially only work within the Create/Update components, is that the case ? I assumed it was meant to be used by itself but if it is not the case we should be ok removing this Wrap here.

yeah, I think we should not allow using UploadImage component outside of create/update components. Probably can get rid of it

veekeys · 2023-12-11T10:52:15Z

packages/modules/create-imodel/README.md

@@ -4,6 +4,28 @@

 Contains components to create or update an iModel.

+### iTwinUI V3 breaking change


This does not look good.
We have N packages using iTwinUI and wanting to migrate to v3 independently.
v3 was advertised to work easily with v2 without any huge changes. Installing alias is a huge change, IMO.
@mayank99 @FlyersPh9, before the big wave of migration started, could you have a look if its possible to make this easier? Cause adding documentation on each package to add some external css to work, does not look good.

it's complicated, we have to make a tradeoff between:

relying on side-effect imports (which causes issues with ordering, duplication and a lot more)

relying on client-side JS to inject styles (which is hacky, doesn't work with SSR, and gives no control to the end user for postprocessing)

providing a full stylesheet (which requires manual work)

the ideal scenario here is that applications will migrate to v3 earlier, because they don't really have to wait for packages anymore

Application cannot migrate to v3 earlier than package. Scope is just bigger compared to the package.

then the application could continue using the old version of all packages that are already compatible with iTwinUI v2.

it's similar to how an application would not consume a dependency that uses react 18 features, until the application itself is updated to use react 18.

in v2, packages had to migrate first, which was a blocker for applications. in v3, the relationship is flipped: applications can migrate to v3 first.

i understand the scope is bigger, but it needs to be done at some point anyway. v2 will not get any new features.

no matter what we do, application developers will always come up with excuses and complaints. we undid all breaking changes in itwinui-react v2, and yet so many of our users are still using v1. it feels like a one-way exchange.

i also don't see npm aliases as a "huge change". it's two lines of code. what am i missing? maybe the biggest concern here is unfamiliarity. we could try documenting this in more detail if that helps. (see current documentation)

Well, if as you mentionned using different aliases causes context to unmatch, this is a huge change because it will likely give weird problems. (Still need to be confirmed though)

Tested in storybook on this branch (removed the styles import in preview.js) (webpack 5, with storybook magic)
Also tested in appui connected tests apps updating imodel-browser-react to 1.2.0 and forcing imodel-browser-react->itwinui-react dependency to 3.1.3-dev.1 through .pnpmfile.cjs (webpack 5, with CRA magic (bentley scripts))

Styles are applied correctly, was coming from 0.13.3, which only required projectId->iTwinId breaking change introduced by 1.0.0. (That was expected)

Tested in web and electron (which uses the same webpacked frontend...) only thing I could see for now is this, which is apparently triggered by a Tile (see other image) but all the tiles are wrapped in a ThemeProvider, so I'm not sure what's going on. I'll look a bit more tomorrow, I'm a bit surprised that the classname is iui-tile and not _iui3-tile...

the "iui-tile" thing is expected — we transform the CSS class at runtime, so don't worry about that.

it's possible that there are two different modules being used (ESM vs CJS maybe?), but if you're having trouble fixing it and the theming works fine anyway, then this error can be ignored because it only happens during development.

Ok, after more research on this, the breakpoint was not on the right spot in the above image, the issues come from NoResults component which is indeed missing the ThemeProvider. Adding it in this PR shoudl fix that last issue.

This is now released for stable use in 3.2.0

Thanks for this, I'll look at reincorporating all the reverted changes !

packages/modules/delete-imodel/src/components/DeleteIModel.tsx

Fix useToaster missing ThemeProvider wrapping

71c9c12

Add `iTwinUI` V3 style import info in readmes. Switch some `iui` classes to their component version. Removed default `theme="inherit"` from `ThemeProviders`

raplemie requested review from williamkbentley and veekeys as code owners December 8, 2023 19:49

veekeys reviewed Dec 11, 2023

View reviewed changes

raplemie added 2 commits December 11, 2023 11:20

Remove unnecessary ThemeProvider in BaseIModel

15b5365

Simplify READMEs

60f7d2e

raplemie mentioned this pull request Dec 13, 2023

Revert iTwinUI v3 #114

Merged

Update to 3.1.0, use 2.x in storybook to validate theme sync

9c514bf

raplemie requested a review from kckst8 as a code owner December 13, 2023 21:04

raplemie added 2 commits December 13, 2023 16:17

Allow itwinui 2 for storybook testing

c4df425

Fix useToaster test issues and removing unneeded mocks

fd98994

raplemie mentioned this pull request Dec 14, 2023

[Manage-version] Fixes named version order and date format in table + reload subrows on create version #112

Merged

mayank99 mentioned this pull request Dec 18, 2023

automatically load css at runtime as fallback iTwin/iTwinUI#1734

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust to iTwinUI V3 breaks. #113

Adjust to iTwinUI V3 breaks. #113

raplemie commented Dec 8, 2023 •

edited

Loading

veekeys Dec 11, 2023

raplemie Dec 11, 2023

mayank99 Dec 11, 2023 •

edited

Loading

mayank99 Dec 11, 2023

raplemie Dec 11, 2023

raplemie Dec 11, 2023

mayank99 Dec 11, 2023 •

edited

Loading

veekeys Dec 11, 2023

mayank99 Dec 11, 2023

raplemie Dec 13, 2023

veekeys Dec 14, 2023

veekeys Dec 11, 2023

mayank99 Dec 11, 2023

veekeys Dec 11, 2023

mayank99 Dec 11, 2023

raplemie Dec 11, 2023

raplemie Dec 14, 2023

mayank99 Dec 15, 2023 •

edited

Loading

raplemie Dec 15, 2023

mayank99 Dec 19, 2023

raplemie Dec 20, 2023

		@@ -4,6 +4,28 @@

		Contains components to create or update an iModel.

		### iTwinUI V3 breaking change

Adjust to iTwinUI V3 breaks. #113

Are you sure you want to change the base?

Adjust to iTwinUI V3 breaks. #113

Conversation

raplemie commented Dec 8, 2023 • edited Loading

Breaking changes...

Add iTwinUI V3 style import info in readmes.

Fix useToaster missing ThemeProvider wrapping.

Switch some iui classes to their component version.

Removed default theme="inherit" from ThemeProviders

Remaining issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayank99 Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayank99 Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayank99 Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raplemie commented Dec 8, 2023 •

edited

Loading

Add `iTwinUI` V3 style import info in readmes.

Fix `useToaster` missing `ThemeProvider` wrapping.

Switch some `iui` classes to their component version.

Removed default `theme="inherit"` from `ThemeProviders`

mayank99 Dec 11, 2023 •

edited

Loading

mayank99 Dec 11, 2023 •

edited

Loading

mayank99 Dec 15, 2023 •

edited

Loading