Web Viewer React Template Updates

[claudiaareneee]

Hello, team!

I have a couple of questions / requests for the web viewer react template.

1. Can we rename the onIModelAppInit callback to handleIModelAppInit to meet react naming conventions? (See App.tsx, Line 131)
2. What all does the viewConfiguration method do? I know it fits the view, but there are other ways to do that. Is it possible to extract the performance monitoring somewhere else? For most beginners, this bit of code is really intimidating. (See App.tsx, Line 95)

I don't mind creating my own PR and contributing, but I want to get feedback first.

Thanks!

[claudiaareneee]

Adding @Josh-Schifter @roopksaini

[ben-polinsky]

Hello.

We can certainly have better docs and expose them in a better way. I know we've had preliminary discussions...

1. I'd argue that onIModelAppInit is just callback and not an event handler and thus shouldn't follow those guidelines.

2. The viewConfiguration method does use ViewerPerformance, but I believe all that is required is to setup performance monitoring is to set the enablePerformanceMonitors prop to true: https://github.com/iTwin/viewer/tree/master/packages/modules/web-viewer-react#required then you can use ViewerPerformance on its own. But I haven't actually tried this out, so if you're seeing something different please let me know!

In terms of what viewConfiguration does, beyond the obvious you pointed out, I'm not quite sure the history and why we're fitting the view this way. I'd ask for a bit of clarification from @aruniverse before asking for a PR to simplify.

Thanks, appreciate the feedback.

[aruniverse]

as per the comments,

  /** NOTE: This function will execute the "Fit View" tool after the iModel is loaded into the Viewer.
   * This will provide an "optimal" view of the model. However, it will override any default views that are
   * stored in the iModel. Delete this function and the prop that it is passed to if you prefer
   * to honor default views when they are present instead (the Viewer will still apply a similar function to iModels that do not have a default view).
   */
  const viewConfiguration = useCallback((viewPort: ScreenViewport) => {

Like ben said, its primarily used for performance monitoring, specifically to determine how long it takes to load all the tiles for a given view, the default view, and to capture it in the profiler.

Like the comment says, feel free to delete it if you don't want/need to use the default performance monitoring we have set up (it doesn't affect performance of the viewer). The fn was added as an example for how to modify the vp. If the user omits this, we will still do the fit to view / performance monitoring as part of the default view configurer we have in https://github.com/iTwin/viewer/blob/dad1ea4580ace8e56ec4a0d77e496fe6a0b52e2d/packages/modules/viewer-react/src/services/iModel/ViewCreator3d.ts#L71

I'd be ok removing this from the template.

[Josh-Schifter]

In my opinion, we could potentially create a dedicated sample that shows how to do performance monitoring but it should not be part of the template. Still, I'd like the template to override the default view, but I'd rather it be done using ViewCreator and not by invoking a tool (and certainly not based on an arbitrary timer delay.

Adding @YashodipD to comment on "just a callback and not an event handler".

[YashodipD]

just a callback and not an event handler

I don't see much of difference between callback and event handler. IMO, Callbacks ensure that the function will not run before the task is completed but will run right after the task is completed. In our case, event handler gets called right after event is occurred.

Detailed explanation about why to follow method naming convention?

In the React world, when we add 'on' prefix to anything that means it is an event. React has derived that convention from HTML. I can give you several example- onChange, onClick, onLoad. etc., It is similar to HTML world. Important thing is React is not HTML. It has JSX and JSX is syntactic sugar.

Let me add text from React official website. I am quoting it here - "By convention, it is common to name event handlers as handle followed by the event name. You’ll often see onClick={handleClick}, onMouseEnter={handleMouseEnter}, and so on." Link to above text - https://react.dev/learn/responding-to-events#adding-event-handlers

onClick - event handleClick - event handler / callback

so, React has this extra edge that we can create our own custom events. I would say that onIModelAppInit is an custom event that we created to handle situation when IModelApp initializes what to do after that. whenever onIModelAppInit event occurs we define what to do, it is defined inside event handler function.

It is recommended to stick to convention when you create custom event and event handler - onIModelAppInit - custom event
handleIModelAppInit - event handler / callback

why is it required to follow the guideline? why this convention is accepted worldwide?

• When we have event and event handler in same component, it does not make a much difference.
• It makes difference when we are working in big project and there are thousands of line of code. We don't know at which level in the component hierarchy event handler defined. If we don't follow the convention it is confusing for the developer. Developer has to scroll up and down to get the definition, it is broken developer experience.
• Now specific to our iTwin viewer template. We say iTwin viewer template is based on React and at the same time we don't follow React method naming convention. It sounds strange to me.

Here I have one small sandbox code which demonstrate the naming convention. See the below screenshots as well. Link to sandbox - https://codesandbox.io/s/event-handler-method-naming-convention-5zy9k8?file=/App.js

Wrong approach -

Right approach -

[YashodipD]

so, I will just quickly summarize.

• It makes developer experience better.
• Easy to differentiate between what is event and what is event handling method.

[ben-polinsky]

I appreciate the thought here. I'm not trying to be difficult, just precise.

I don't see much of difference between callback and event handler.

All event handlers are callbacks, but all callbacks are not event handlers. If I pass a function as a parameter to a function, and that parameter gets called at some point during the function's execution, it's not an event handler, it's a callback - there are no events involved.

An event handler is literally about handling an Event, which are specific things in JavaScript. Callbacks don't just do what you described, they're any function that gets passed and "called back" at some point later.

I would think it would be more confusing for developers to see a handle* method, and not have it handle any event.

Following your proposed example:

handleIModelAppInit - event handler onIModelAppInit - this does not exist. Because there is no onIModelAppInit event.

All the examples you show deal with actual events.

[YashodipD]

First let me be clear that I described onIModelAppInit as custom event. Not like a regular event. I see it as event because I see that on prefix is added to that. If core team did not add prefix on. I would not be misguided.

Now 2 possibilities are here

• As you say it is not an event and it does not exist then do not name it like it is an event because that's what we see and learned from normal HTML and React world. Just rename onIModelAppInit to IModelAppInit. So, people like me do not confuse that it is an event.
• If you accept that it is custom event then please follow method naming convention.

I see it as custom event because that will happen every time whenever IModel initialization is finished. Now it is up to user that if he really wish to leverage that event. Not mandatory.

[ben-polinsky]

CustomEvents are also a thing in JS, so I wasn't quite sure what you meant.

I do see what you're saying. The function is called when something happens, so that's event-like behavior. And using on as a prefix is misleading according to standards.

I'd prefer something like afterIModelAppInit? Because functions generally seem better as verbs.

[YashodipD]

If we are saying that Bentley's CRA is react-based. Then it makes sense to follow react like convention. If we say something like afterIModelAppInit. Still, it seems like event, but Bentley has invented something new. It will become another topic discussion for (current + new) Bentley technology users/ devs. I would prefer to go with the flow of React naming convention and outside world is much more familiar. Just rename the handler function.

There is one more event - onIModelConnected. It will become afterIModelConnected.

And I already see questions coming from all the external devs (existing + new). what does this after means / do? I will be explaining something like when something happens in the viewer, it is going to call that callback which we provided. Doesn't it sound like event?

[aruniverse]

I appreciate the back and forth and I shouldve chimed in earlier when Ben first responded, but we will NOT be changing the name of the onImodelAppInit or onIModelConnected props; that's an unnecessary breaking change. Those props have existed since the very early days of the viewer, back when we were under the old scope and package name. A change like this would be massive and effect practically every single user of the viewer for very little benefit.

Please feel free to open a PR to remove/update the default viewConfiguration, but other than that we will not be accepting any changes for renaming the props

[Josh-Schifter]

Of course renaming the props is out of scope. However, I see no downside to renaming the 'handler' method.

[YashodipD]

Since the beginning I am insisting that we should rename the handler method in the template.

As I earlier said in the thread there are only 2 possibilities either change the name of prop or follow the react method naming convention accordingly.

So renaming prop is out of scope. then we are left with method naming convention choice and it is easy to follow. onIModelAppInit - custom event handleIModelAppInit - event handler / callback

I would recommend everyone to follow above convention (why explained earlier here ) then, because I do not want to appear naive in front of react people. It is very easy to follow method naming convention.

Also, it will make developer experience better.

[YashodipD]

I was going through GitHub issue as usual. I noticed one issue ( https://github.com/iTwin/viewer/issues/138) reported by Ali Aslam.

Everyone see it as event so I hope you will accept other renaming related changes as well.