A component nested in another as a pointer is always nil

[filmil]

I stepped on this while trying to migrate this piece of software from go-app v6 to v9.
https://github.com/grahamjenson/bazel-golang-wasm-proto/pull/4/files#diff-2afb4d11c8069bc5fb58b2d00e2e0c9c3cb8b8bb926bf648ed7b95d856655af7R14

In short, components are declared as follows (details in the PR):

struct Manager {
  app.Compo

  SearchBar SearchBar
  InstanceTable InstanceTable
}

struct SearchBar {
  app.Compo

  Manager *Manager
}

struct InstanceTable {
  app.Compo
  Manager *Manager
}

The code gets initialized in main.go:

        // ...
	manager := Manager{
		SearchBar:     SearchBar{},
		InstanceTable: InstanceTable{},
	}

	manager.SearchBar.SetManager(&manager)
	manager.InstanceTable.SetManager(&manager)

	log.Printf("main: manager: %p, %+v", &manager, manager)

	app.Route("/", &manager)
	app.RunWhenOnBrowser()

What I observe on the browser side is that, despite calling SetManager(...) on both SearchBar and InstanceTable, the value of their respective .Manager field is always nil. Logging some addresses of SearchBar and InstanceTable, I noticed that the addresses of the respective instances are not the same as the addresses of the instances composed into Manager.

This of course fails with a nil dereference, when SearchBar tries to refer to its nil manager. See https://github.com/grahamjenson/bazel-golang-wasm-proto/pull/4/files#diff-a2c869579f3c6ab043908c5904a5dfd0264e7b9717e08c14edac23a29b03cdf9R43

It seems as if instances of SearchBar and InstanceTable are being copied even if I don't want them to be copied, and the respective pointers aren't copied. Any ideas why this is happening? This is only an issue in v9, it seems that v6 is fine with the general approach.

It's not out of the question that I'm doing something wrong. The example apps in go-app don't treat this at all- there's just a basic "hello world", and others don't have app.Compo hold pointers to other UI elements. What gives?

Thank you for looking at this!

[filmil]

Hm, this seems similar to #559

[oderwat]

As documented, app.Route("/", &manager) is not using the value of manager, but it's type.

You need to initialize its values in OnMount() or try the new RouteFunc() for initializing the component.

[filmil]

Ow. Thanks for explaining and the doc pointer. I now understand better what is going on. But now to me it seems like a bad API, since it is easy to use incorrectly and astonish the user. A good API should make incorrect use impossible.

Some options.

1. app.Route(string, SomeOtherUniqueType), and app.Composer should have a app.Composer.GetRoutingType() SomeOtherUniqueType, to ensure that you can't insert an initialized struct and expect it to work.
2. Another option is to remove app.Route and only use app.RouteFunc. It says more clearly what is actually needed, which is a factory, not a struct pointer.

Either way, you don't even need to document the peculiar behavior where if you do app.Route("/", &someStruct), you actually don't get to use someStruct that you passed in.

[oderwat]

It needs to be the correct type, though. You cannot hand over SomeOtherUniqueType and then create the needed type from it. I think there is no way to give "only" a type to a Go function. I think @maxence-charriere may consider using (*Component)(nil) in his examples and maybe even give a runtime error when the parameter given is not nil. But that is not backward compatible at all and feels awkward to me.

The RouteFunc() variant is cool, but it is also not backward compatible. It also means that you have to add a function that returns an empty struct in numerous instances. Still, I was surprised that he did not take the PR earlier. Maybe Maxcence is just focused a lot on his ideas, and that is fine with me. He did a lot of work, and we use it for free (although that we have sponsored him for some time now).

The right place to initialize your components actually should probably always be OnMount() (or OnInit() for some use cases. The ´RouteFunc()variant also may not work as expected, then the type that is returned is the same as the one for another route. Updating nodes works different, then one thinks at first. I did not try if that is also the case withRouteFunc()` yet (we only have one route in nearly all of our applications).

That you want to initialize your component before the route is called and therefor used, is also usually not very likely. Think of having 10 routes. You do not want to have all of these components being values.

You may want to read #730 and check out "mountpoint" (and the example) from https://github.com/metatexx/go-app-pkgs

P.S.: It is also a problem you have just once in a life time :)

[filmil]

I think there is no way to give "only" a type to a Go function.

Sure you can: https://go.dev/play/p/SYSjyVvp1ui

The idea here is to prevent the user from inadvertently passing in f or &f where only a type is going to be used.

He did a lot of work, and we use it for free (although that we have sponsored him for some time now)

Absolutely. And I'm happy to give credit where credit is due. I still think there is room for API improvements nevertheless.
(Happy to contribute a PR, but would like to know if the project owner would accept this before I invest the time.)

The right place to initialize your components actually should probably always be OnMount() (or OnInit() for some use cases.

You are probably right. But this is not relevant from the perspective of the app.Route() API

You do not want to have all of these components being values.

Agreed; however, it's still a good idea to prevent API misuse. To wit, it's easy to know this in principle. It is a different thing to be cognizant of the issue at all times, especially when (1) there may be hundreds of components you wrangle; (2) the composition pattern is the fundamental idea to software engineering and should work correctly all the time; and (3) the resulting errors can be really confusing, since if you refer to an internal pointer field you get something like app.Div().Body(topLevelWidget.LowLevelWidget(), where topLevelWidget.LowLevelWidget==nil yields a confusing UI where nothing is rendered and yet no error is printed (since a nil widget seems to be OK at present).

Lots of footguns above. Avoid footguns.

P.S.: It is also a problem you have just once in a life time :)

If you can have it zero times in a lifetime, it would surely be better?

[oderwat]

https://go.dev/play/p/SYSjyVvp1ui

Well, that is a clever trick with the (limited) generics support and does not look very appealing to me. Even this does not take a type anyway. It takes an interface with a function that returns a typed nil value. And to explain it to a user, you need to explain that you made it harder to give a component in the route because people failed to read the documentation. I am uncertain if I advocated this.

If you can have it zero times in a lifetime, it would surely be better?

Sure, and I did the same error as you (not reading the documentation of functions I used).

As I said, I think the example could be clearer to point it out that you just give an example of the type. But I am not @maxence-charriere, and he will have his own idea about it, he always has :)

[filmil]

As I said, I think the example could be clearer to point it out that you just give an example of the type.

I suppose so. Prior to reporting this bug, I stared at that sentence in the original documentation and I didn't understand what it meant. Not until you provided the context.

But I am not @maxence-charriere, and he will have his own idea about it, he always has :)

Fair point. There's enough material here to weigh the pros and cons of the issue, and I'm happy to leave it at that. I remain grateful that this project exists.