Some feedback - my initial thoughts

[daniel-v]

I dag into what you did here and overall, I really like some of the things I see. With my appreciation to the effort you put into making this happen, there are some areas that I think could benefit from a bit more attentive coding love :)

GJ, so far, I hope you keep on going!

Statics - testability - multiple apps

1. _registeredWidgetObjects is static in framework.dart - multiple instances an app is a no-go?
   • you are protecting against multiple FW instances with checking _isInit value
   • An app should probably run in its own zone to make sure that errors from one doesn't break others, in case you enable multiple app to run on the same page (there are some nice use-cases for this)
   • Maybe, a similar "bootstrapping" mechanism can be used as seen in Flutter runApp(RadApp, someOptions) to init logging, debug tooling, zones, etc.
2. statics effectively prevent tests to mock out pieces of the FW - make it very tedious

Navigator and routes

This is tough one. Angular didn't get it right the first time, nor did Flutter. No surprises here :)

2. window.addEventListener("popstate" could be written as window.onPopState.listen
   • listeners should be disposed when app is disposed
3. Navigator doesn't seem to react to manual command in console history.pushState({}, '', 'another')
   • this prevents being able to react to external changes, maybe 3rd party libraries
4. In local development, using path based routing is cumbersome. In debug mode, it should be permissible to use hash routes. eg. localhost:8080/#/home
5. sitemap is still a thing, there should be a mechanism in place to extract route information
   • one thing that pops into mind is compile time const expressions for routes
6. Routes should be allowed to be loaded async with deffered as import statements
   • AsyncRoute subclass maybe?
   • or maybe instead of page, there could be a AsyncWidgetBuilder callback for the Route with FutureOr return value?
7. There is a seemingly arbitrary restriction what a path might look like for a Route: RegExp(r'^[a-zA-Z0-9_]+$'). Is there a reason for this?
   • with that routes such as this would be not possible /blog/some-blog-entry-title-sanitized'
8. Is there a way to capture paths as params to a Route? eg. /blog/:slug:

Events and event handling

I noticed that there isn't really a consistent handling of events across the widgets. Most, if not all, have an onClickEventListener and there is GestureDetector widget. There are important use-cases that I can't cover with these 2.

There is one workaround I found: using a stateful widget where I get to access the DOM element from initState and go underneath the FW.

  late StreamSubscription sub;

  void initState() {
    sub = element.onMouseEnter.listen((event) {
      print('${(event.currentTarget! as Element).text}');
    });
  }

  void dispose() {
    sub.cancel();
  }

While this works, it feels out of place. A way to automagically handle stream subscriptions and their disposals would be beneficial.

Maybe something along these lines?

class MouseEnterSample extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Handle(
      events: {
        'mouseEnter': enterHandler,
        'my-custom-event', customHandler,
      },
      child: MyButton,
    );
  }
}

[hamsbrar]

Hi Daniel,

I'm very glad that you took your time to give a comprehensive feedback. Before I began making any comments, please note that everything is definitely not the best. I'm not familiar with innards of other FWs so I tried different approaches, and picked the ones that sounds about right to me. I'm totally open to improvements and corrections.

Static, Multiple instances

I think both are related. Earlier, every widget was responsible for rendering itself and calling build on its childs. /core was intended to provide set of global functions(including methods that build, update widgets). This was clearly an design issue but I carried on with it because that was easier to do at that time.

Framework and Router can lives in an instance if widgets aren't responsible for rendering their child or calling rebuild for themselves. Gradually I was able to take away these responsibilities from all widgets except Navigator, ListView, and StatefulWidget. Children of widgets like ListView are still rendered/updated only when their state allows them to. Working this out requires some sort of scheduler that these widgets can call, or some sort of DI from framework side, or moving their state inside Framework. I'll think more about it, but if you've any ideas/thoughts, feel free to pitch in.

Navigator and routes

1. window.addEventListener("popstate" could be written as window.onPopState.listen

   This is definitely more clean. I just have to check whether event stream is broadcast-able.

2. Navigator doesn't seem to react to manual command in console history.pushState({}, '', 'another')

   Right now, Navigator is listening for only pop state events, which aren't triggered through console(as per search results).

   From mozilla docs: Note that just calling history.pushState() or history.replaceState() won't trigger a popstate event. The popstate event will be triggered by doing a browser action such as a click on the back or forward button (or calling history.back() or history.forward() in JavaScript).

3. In local development, using path based routing is cumbersome. In debug mode, it should be permissible to use hash routes. eg. localhost:8080/#/home

   That's true, I'm using proxy for developing my projects but I get it proxy isn't ideal.

4. sitemap is still a thing, there should be a mechanism in place to extract route information

   I've never really worked with site-maps so I've no idea about this part. I'll take a look and see what can be done.

5. Routes should be allowed to be loaded async with deffered as import statements

   FW builds only one route at a time(the one that is opened/or where user is landed). So as I understand it, problem is with widget constructors:

   routes: [
       Route(name: 'home', page: HeavyConstructor1()),
       Route(name: 'home', page: HeavyConstructor2()),
   ]

   And, here's how I deffered heavy constructions in the past:

   routes: [
       Route.lazy(name: 'home', page: () => HeavyConstructor1()),
       Route.lazy(name: 'home', page: () => HeavyConstructor2()),
   ]

   If I'm not wrong that'll do it? or is there some other reason for having an AsyncRoute?

6. There is a seemingly arbitrary restriction what a path might look like for a Route: RegExp(r'^[a-zA-Z0-9_]+$'). Is there a reason for this?

   I think I was worried about slashes at the time of writing this. This restriction is definitely more strict than required so it should be fixed to allow more characters. We can get rid of this completely by encoding path but there's more to it. For example,

   Route(path: "/home/profile"),

   Here people might assume that they've registered a nested route but that's not true. Without above restriction, they'll see %2Fhome%2Fprofile popping up in their address bar, and I don't think this will be a good way to introduce someone to a new FW.

   With an error, they'll think about "how to do nested routing" in this FW and eventually they'll find the correct way to do it:

   Route(
       path: "home", 
       page: Navigator(
           routes: [
               Route(path: "profile", page: ...),
           ]
       ), 
   );

7. Is there a way to capture paths as params to a Route? eg. /blog/:slug:

   Unfortunately, Router matches only exact segments. I'm not sure why you want to do that?

   For example, you can have a Route(name: 'blog') and if user is on /blog/<some-value>/:

   Navigator.of(context).getValue("blog"); // -> '<some-value>'

Events and event handling

I think on click events are used far more often than any other events. The reason GestureDetector exists has to do with nested widgets that are listening for the same event. GD provides hit test behavior that can be used to control event bubbling and propagation in such cases.

I think the last example you gave is far more easy to use than any other widget ever will be. This looks very clean but we've to use a bit more descriptive name than handle. Although events should be handled by widgets where they are supposed to be(HTML widgets) but adding more work inside HTML widgets will incur performance degradation. So having a different widget sounds good to me.

Regards,

/erlage

[hamsbrar]

ah forgot to mention, we could add more event types to GestureDetector as well.

[daniel-v]

Children of widgets like ListView are still rendered/updated only when their state allows them to.

Could you expand on this a little more? What does it mean "when their state allows them to" and how is it different from a regular Widget?

I just have to check whether event stream is broadcast-able

Indeed, DOM events are broadcast streams. Also, a package I very much enjoy using is rxdart which has some really nice constructs for managing streams. (though your initial take to have as few external dependencies as possible is probably a good enough reason not to use it)

Note that just calling history.pushState() or history.replaceState() won't trigger a popstate event

Indeed! Never wrote a navigator before. A little bit of research and I'm sure we could tackle this. The URL of a page is a "shared resource". If multiple app instances were to be supported, it should be supported that Navigators can react to external programmatic changes too.

That's true, I'm using proxy for developing my projects but I get it proxy isn't ideal.

For a decent sized project, esp. with a BE, this will become a significant enough hindrance to avoid it. Initially, that is exactly how my projects were set up (though back then we used our own implementation). This is good enough for now though! Definitely not the first thing I would want to work on.

I've never really worked with site-maps so I've no idea about this part. I'll take a look and see what can be done.

The sitemaps themselves are not too exciting. Just a way to tell search engines where to find and what. Also not something that is a must have, most SPAs FWs have no first-class support for it. Not a deal breaker if cannot reasonably be done. Why it is still important to is because it can influence how Navigator and Routes are designed.

If I'm not wrong that'll do it? or is there some other reason for having an AsyncRoute?

That will do it - mostly.

If this piece of code works, we have full support:

import 'package:something/aPage.dart' deferred as aPage;
routes: [
    Route.lazy(name: 'home', page: () async {
      await aPage.loadLibrary();
      return APageWidget();
    }),
]

Note: there is an extra async step. Loading the library itself that contains definition for APageWidget().

This looks very clean but we've to use a bit more descriptive name than handle

Agreed :)

adding more work inside HTML widgets will incur performance degradation

Not at all worried. Performance is of little concern to me at this stage. Premature optimization and all that jazz. :)

[hamsbrar]

What does it mean "when their state allows them to" and how is it different from a regular Widget?

Two important things that every widget do:

1. Tells framework the type of HTML tag they want in document.
2. Optionally provide a list of their immediate child widgets.

FW creates the requested HTML element and give widget a chance manipulate the element by calling render on widget's render object. Then FW starts building child widgets if widget has provided a non-empty list of childs.

Regular widgets(e.g a Span widget) don't want to know/control how/when their child widgets are built so they simply provide their child widgets in list. But widgets with state wants to build/update their child widgets only when a particular event occurs. These builds are almost spontaneous and exposing Framework.* was the easiest thing. Since everything in Framework is static, widgets can start build/rebuild process anywhere in the tree. I guess this is the only thing we've to fix.

If multiple app instances were to be supported, it should be supported that Navigators can react to external programmatic changes too

I think the reason why history.pushState doesn't fire a pop state event is obivious. Telling whether a pop state event is occured from history.pushState or a browser action is hard if not immpossible. This needs some work if we want to allow multiple instances of Routers as well(which we should). In particular, each router instance has to ignore events that aren't relevant or captured by the other router else opening route in one router will force the others to switch to their default route.

there is an extra async step

I gotcha. AsyncRoutes are a different thing. Pardon my ignorance:)

That's true, I'm using proxy for developing my projects but I get it proxy isn't ideal.

I've spent good amount of time to get the web debuggers work in proxy mode. I don't know why I was being stupid all the time but thanks for bringing this up. Just realised that all I needed was to prefix path with a #, and we already have the ability to do that:

RadApp(
    routingPath: '#',
)

We can make this implicit in dev mode if you agree on that.

[hamsbrar]

First of all, thank you for working on multiple instances.

Here's update on remaining things:

• AsyncRoute: Possible syntax that I can think of:

  routes: [
  AsyncRoute(
    name: 'some_route',
    page: () async {} // -> Future<Widget>
    preloader: Text('loading'), // -> Widget?
    onError: 'whoops', // -> String?
  ),
  Route(name: 'whoops', page: Text('Something went wrong')),
  ]

• Hash based routing: I saw that just prefixing routingPath with a # doesn't solve anything, so I'll work on it.
• A widget with more events: #7

I think it'd be great if we ignore following things for now:

• Routers listening to external events.
• Site maps (I need to learn more about ways they can be generated).

thanks again.