Refac js components

[jkulhanek]

This PR implements the following:

• Split components into individual tsx files
• Stores attributes and values inside components' configs
• Renames initial_value->value for add_gui_* methods

The goal is gradually moving towards a unified API to add new components.

@brentyi - please let me know what you think.

[jkulhanek]

@brentyi , if you like it, can you please help me with the mypy error? My limited python knowledge is not enough to solve the issue :/

[brentyi]

Need to find time to go through this in-depth, but looks good from a skim! Also happy to look into the mypy error...

The main thing I'm uneasy about is the initial_value= => value= API change in the add_gui_*() methods: is this about consistency with other properties / attributes? Would it make a refactor a la #161 much easier down-the-line? Going through with this would mean either an eventual breaking change or indefinitely supporting both names, both of which would be nice to avoid.

[jkulhanek]

The rename is both for consistency and to make #161 easier. If we decide to use code generation for #161 then it wouldn’t matter much. For pure python typing this would help. However, I understand that introducing a breaking change may not be preferable for you. I can undo the name change if you want?

[brentyi]

Okay, let's switch it back. Reasoning: it seems unlikely we'll converge to a ParamSpec-based refactor we're happy with, and value is a little bit special (it can be updated by the client) so I'm okay with the inconsistency.

[jkulhanek]

Okay, let's switch it back. Reasoning: it seems unlikely we'll converge to a ParamSpec-based refactor we're happy with, and value is a little bit special (it can be updated by the client) so I'm okay with the inconsistency.

Why is value special? You can change other properties as well: visible, disables, etc., no? I was also thinking about the following use case: in future perhaps we want to enable property binding to make the app faster (bind a property to another component's property).

[jkulhanek]

I undid the api change.

[jkulhanek]

But, initial_value->value is still done in messages! Is that ok?

[brentyi]

Why is value special? You can change other properties as well: visible, disables, etc., no?

The difference I was alluding to is that everything in a GuiAdd* message (disabled, hint, initial_value, etc) can only be changed via an assignment from the server. Whereas the value can also be updated by the client.

For the internals and in messages, there are minor but important semantic differences between what it means when we write value and when we write initial_value: when we send a GuiAdd[...] message to update just a single property, for example for dropdowns:

viser/src/viser/_gui_handles.py

Lines 309 to 328 in abf6484

	@options.setter
	def options(self, options: Iterable[StringType]) -> None:
	self._impl_options = tuple(options)
	if self._impl.initial_value not in self._impl_options:
	self._impl.initial_value = self._impl_options[0]
	self._impl.gui_api._get_api()._queue(
	GuiAddDropdownMessage(
	order=self._impl.order,
	id=self._impl.id,
	label=self._impl.label,
	container_id=self._impl.container_id,
	hint=self._impl.hint,
	initial_value=self._impl.initial_value,
	options=self._impl_options,
	)
	)
	if self.value not in self._impl_options:
	self.value = self._impl_options[0]

we re-send the original initial_value, but this won't overwrite whatever the client has stored if it updates the value of the input. This was convenient at the time because it lets the server update input properties without worrying about things like race conditions. Does this make sense?

[jkulhanek]

I understand the motivation. I still think doing partial updates (updating only changed properties) would have been safer and more principled way to do it. In the case where you set disabled/visible attributes from the client to server, they are not synchronised from server to client, right? Wouldn't it be better to treat all props the same?

[brentyi]

I still think doing partial updates (updating only changed properties) would have been safer and more principled way to do it.

Yeah, I agree. I just didn't bother with the initial implementation because sending the full list of properties was much simpler to implement.

Did you have an implementation in mind for doing a partial update? It seems like we could do one of:

• Reuse the same message type, but make all of the properties optional so we only need to send what's needed? Use a TypedDict with total=false?
• (in the .options example above) Add a new SetGuiDropdownOptions message?
• Use a generic SetGuiProp message?

I think the internal initial_value => value name change makes sense if we go with one of these options. I'd be slightly opposed to naming a field value that's actually functioning as an initial_value (eg not overwriting the client-side value when sent from the server).

In the case where you set disabled/visible attributes from the client to server, they are not synchronised from server to client, right?

That's correct, although I was never planning on adding mechanics to set these attributes from the client. If we added some kind of property binding like you mentioned this might become possible, although I'm skeptical the complexity tradeoff here.

[jkulhanek]

I think SetGuiProp is the easiest to implement, although a bit less safe on the python side. However, if eventually the client handles are generic, there should be no risk of introducing bugs this way. The benefit is that the current messages could stay the same.
I believe adding options wouldn't scale that well. Perhaps a generic approach is preferable?

Which one would you prefer?

[jkulhanek]

Or perhaps we can have typeddict props (ButtonProps, TextInputProps) and AddGuiComponent, UpdateGuiComponent messages?

[jkulhanek]

Please let me know what you think now.

[brentyi]

(edit: now fixed) getting closer, need to fix some performance regressions

in this branch:

Screen.Recording.2024-02-06.at.1.59.12.AM.mov

on main:

Screen.Recording.2024-02-06.at.2.00.31.AM.mov